Closed Bug 1277433 Opened 8 years ago Closed 8 years ago

Use discrete animation for appropriate css alignment properties

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

(
URL
)

Details

Attachments

(11 files, 5 obsolete files)

Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.
58 bytes, text/x-review-board-request
dholbert
: review+
heycam
: review+
Details
Bug 1277433 - Part 2: Add tests for CSS Alignment.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1277433 - Part 3: Add tests for CSS Flexbox.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1277433 - Part 4: Add CSS Animation tests.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1277433 - Part 5: Replace eStyleAnimType_EnumU8 to eStyleAnimType_Discrete.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1277433 - Part 6: Add tests that were eStyleAnimType_EnumU8.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1277433 - Part 7: Remove eStyleAnimType_EnumU8 related codes.
58 bytes, text/x-review-board-request
birtles
: review+
Details
flexbox-1277433-6.patch
27.69 KB, patch
birtles
: review+
Details | Diff | Splinter Review
discrete-AnimValuesStyleRule.html
314 bytes, text/html
Details
discrete-HTMLBodyElement-BodyRule.html
221 bytes, text/html
Details
discrete-nsMappedAttributes.html
365 bytes, text/html
Details 
      No description provided.
Keywords: meta
Keywords: meta
Assignee: nobody → daisuke
I think dbaron, heycam, or dholbert might be a better reviewer for part 1.
Comment on attachment 8760600 [details]
Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.

Hello Daniel,
Nice to meet you!

Couldn't you review this patch?

Thanks,
Daisuke
Attachment #8760600 - Flags: review?(bbirtles) → review?(dholbert)
Comment on attachment 8760600 [details]
Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.

https://reviewboard.mozilla.org/r/58138/#review55110

Thanks for the patch! This is nearly ready, just a few notes.

First, please update the commit message to mention CSS Alignment instead of CSS Flexbox, since that's how these properties are categorized now:
 https://drafts.csswg.org/css-align/#property-index
 

Also: just to be sure I understand, could you explain in a bug comment here what the intended effect of this EnumU8 change is? I suspect we currently reject CSS animations on these properties, and your patch will make us animate by swapping between start/end values 50% of the way through the animation -- is that correct? (I seem to recall that we want to move in that direction for all un-animatable properties, but I'm not 100% sure.)

::: layout/style/nsStyleStruct.h:1710
(Diff revision 1)
> -  uint16_t      mAlignContent;          // [reset] fallback value in the high byte
> -  uint8_t       mAlignItems;            // [reset] see nsStyleConsts.h
> -  uint8_t       mAlignSelf;             // [reset] see nsStyleConsts.h
> -  uint16_t      mJustifyContent;        // [reset] fallback value in the high byte
>    uint8_t       mJustifyItems;          // [reset] see nsStyleConsts.h
>    uint8_t       mJustifySelf;           // [reset] see nsStyleConsts.h

Please make this bug's same changes to "mJustifyItems" and "mJustifySelf", too. Those properties are in the same category as all the others you're changing -- they're under the purview of the "CSS Alignment" spec.  (The ones you've changed were originally added in CSS Flexbox, but they were promoted & expanded into CSS Alignment.)

This will probably mean you don't need to mess with Constructor initialization-list reordering after all.
Attachment #8760600 - Flags: review?(dholbert) → review-
(Please include tests for "justify-items" and "justify-self" alongside your other tests in "Part 2", as well.)
Comment on attachment 8760602 [details]
Bug 1277433 - Part 3: Remove trailing whitespace from nsStyleStruct.cpp.

https://reviewboard.mozilla.org/r/58142/#review55138

And Part 3 looks good [/me steals review on that one], except for one thing -- I'd suggest you update the commit message to be clearer. Right now it says:
> Bug 1277433 - Part 3: Trails whitespaces. r?birtles

Please update to something which more clearly describes the change, like:
 Bug 1277433 - Part 3: Remove trailing whitespace from nsStyleStruct.cpp. r=dholbert

r=me on Part 3, with that fixed.
Attachment #8760602 - Flags: review+
Comment on attachment 8760602 [details]
Bug 1277433 - Part 3: Remove trailing whitespace from nsStyleStruct.cpp.

Clearing review request for part 3 since dholbert has already reviewed this. (Unfortunately MozReview doesn't yet allow anyone other than the patch submitter to change the reviewer so I think you'll have to update the review flags in MozReview yourself if you want this to be landable with auto-land.)
Attachment #8760602 - Flags: review?(bbirtles)
Comment on attachment 8760601 [details]
Bug 1277433 - Part 2: Add tests for CSS Alignment.

https://reviewboard.mozilla.org/r/58140/#review55232

I think we can make this a little simpler and more flexible, perhaps by passing function objects. Perhaps we really need to add a few more properties to the test that use different animation types, however, so we can see how well this approach scales. In particular, it would be good to add all the properties in green in [1]

[1] https://docs.google.com/spreadsheets/d/1apb5mEiom_yFieSmm19TTcMd3X_S5VcLyGHJcB6i0i8/edit#gid=0

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:15
(Diff revision 1)
> +<div id="log"></div>
> +<script>
> +"use strict";
> +
> +var AnimType = {
> +  Discrete: 1

Perhaps we can make the value here 'discrete'? No need to use an integer?

Then perhaps we can also add a comment like "Parameters: 'values' - a list of keywords that we should perform discrete animation between"

Or is it pairs of values?

For example, for 'flex-basis' we need to test that we use discrete animation between '200px' and 'auto'.

(Alternatively, I wonder if we even need an enum or whether we could just have a comment that describes the recognized keywords, e.g. 'discrete')

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:19
(Diff revision 1)
> +  "align-content": [
> +    { type: AnimType.Discrete, values: ["flex-start", "flex-end"] }
> +  ],
> +  "align-items": [
> +    { type: AnimType.Discrete, values: ["flex-start", "flex-end"] }
> +  ],
> +  "align-self": [
> +    { type: AnimType.Discrete, values: ["flex-start", "flex-end"] }
> +  ],
> +  "justify-content": [
> +    { type: AnimType.Discrete, values: ["baseline", "last-baseline"] }
> +  ]

I think we should add all the properties in this spec. Or were you planning on doing that in a separate patch?

I'd like to see how other type of animation are handled so we can determine if this is a good structure or not.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:41
(Diff revision 1)
> +      switch (testcase.type) {
> +        case AnimType.Discrete: {
> +          expected = [testcase.values[0], testcase.values[0],
> +                      testcase.values[1], testcase.values[1],
> +                      testcase.values[1]];
> +          values = testcase.values;
> +          break;
> +        }
> +      }

This should have a default branch so we can assert if we get the wrong type of animation. But maybe, even better still, we could just avoid having types and do something similar to the transitions test case and pass in function objects.

e.g.

"align-content": [
    discrete('flex-start', 'flex-end')
],
"flex-basis": [
    lengthPercentageCalc(),
    discrete('100px', 'auto')
]

Where discrete() is something like:

function discrete(a, b) {
  return function (elem) {
    ... do test
  };
}

Or maybe even:

function discrete(a, b) {
  var testFunc = function (elem) {
    ... do test
  };
  testFunc.animationType = 'discrete';
  return testFunc;
}

(So we can display the animation type in error messages)

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:56
(Diff revision 1)
> +      assert_equals(getComputedStyle(div)[idlName], expected[0],
> +                    "The value should be " + expected[0] + " at 0% progress");
> +
> +      animation.currentTime = 250;
> +      assert_equals(getComputedStyle(div)[idlName], expected[1],
> +                    "The value should be " + expected[1] + " at 25% progress");
> +
> +      animation.currentTime = 500;
> +      assert_equals(getComputedStyle(div)[idlName], expected[2],
> +                    "The value should be " + expected[2] + " at 50% progress");
> +
> +      animation.currentTime = 750;
> +      assert_equals(getComputedStyle(div)[idlName], expected[3],
> +                    "The value should be " + expected[3] + " at 75% progress");
> +
> +      animation.currentTime = 1000;
> +      assert_equals(getComputedStyle(div)[idlName], expected[4],
> +                    "The value should be " + expected[4] + " at 100% progress");

I wonder if we need to test all these points?

Perhaps we can make a general function that takes:

* the animation to seek
* property-name
* an array of sample points like:
   [ { time: 0.25, expected: '200px' },
     { time: 0.5, expected: '400px' } ]

And then the individual test functions (like discrete() etc.) could call that?

Hopefully that would remove some duplicated code from below.
Attachment #8760601 - Flags: review?(bbirtles)
https://reviewboard.mozilla.org/r/58138/#review55110

Thank you for the reviewing!

Okay, I'll update the commit message and add justify-items and justify-self of CSS Alignment in this patch.

Also, yes! It's corrent. This is the spec.
https://w3c.github.io/web-animations/#discrete-animation-type-section
And, this patch does not affect to CSS Transition.
https://lists.w3.org/Archives/Public/www-style/2016May/0121.html

> Please make this bug's same changes to "mJustifyItems" and "mJustifySelf", too. Those properties are in the same category as all the others you're changing -- they're under the purview of the "CSS Alignment" spec.  (The ones you've changed were originally added in CSS Flexbox, but they were promoted & expanded into CSS Alignment.)
> 
> This will probably mean you don't need to mess with Constructor initialization-list reordering after all.

Okay!
(In reply to Daniel Holbert [:dholbert] from comment #9)
> (Please include tests for "justify-items" and "justify-self" alongside your
> other tests in "Part 2", as well.)

Okay!
I'll modify to test for CSS Alignment properties in this patch.
Also, add one more patch (Part 3) to test for all CSS Flexbox properties.
(In reply to Daniel Holbert [:dholbert] from comment #10)
> Comment on attachment 8760602 [details]
> Bug 1277433 - Part 3: Trails whitespaces.
> 
> https://reviewboard.mozilla.org/r/58142/#review55138
> 
> And Part 3 looks good [/me steals review on that one], except for one thing
> -- I'd suggest you update the commit message to be clearer. Right now it
> says:
> > Bug 1277433 - Part 3: Trails whitespaces. r?birtles
> 
> Please update to something which more clearly describes the change, like:
>  Bug 1277433 - Part 3: Remove trailing whitespace from nsStyleStruct.cpp.
> r=dholbert
> 
> r=me on Part 3, with that fixed.

Okay, Thanks!
Review commit: https://reviewboard.mozilla.org/r/58440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58440/
Attachment #8760600 - Attachment description: Bug 1277433 - Part 1: Use discrete animation for appropriate flexbox properties. → Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.
Attachment #8760601 - Attachment description: Bug 1277433 - Part 2: Add tests. → Bug 1277433 - Part 2: Add tests for CSS Alignment.
Attachment #8760602 - Attachment description: Bug 1277433 - Part 3: Trails whitespaces. → Bug 1277433 - Part 3: Remove trailing whitespace from nsStyleStruct.cpp.
Attachment #8761083 - Flags: review?(bbirtles)
Attachment #8760600 - Flags: review- → review?(dholbert)
Attachment #8760601 - Flags: review?(bbirtles)
Comment on attachment 8760600 [details]
Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58138/diff/1-2/
Comment on attachment 8760601 [details]
Bug 1277433 - Part 2: Add tests for CSS Alignment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58140/diff/1-2/
Comment on attachment 8760602 [details]
Bug 1277433 - Part 3: Remove trailing whitespace from nsStyleStruct.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58142/diff/1-2/
Comment on attachment 8760600 [details]
Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.

https://reviewboard.mozilla.org/r/58138/#review55482

::: layout/style/nsStyleStruct.h:1708
(Diff revision 2)
>    mozilla::StyleBoxSizing mBoxSizing;   // [reset] see nsStyleConsts.h
> -private:
> -  friend class nsRuleNode;
>  
> +public:
>    uint16_t      mAlignContent;          // [reset] fallback value in the high byte

Hmm... So on further reflection, I don't think we want to make these members public, actually. (That's the main change that you're currently making to this file.)

They're currently "private" for a reason -- most code should only access them via the accessors, which take care of masking away "fallback values" that are encoded into their high bytes, and which also take care of resolving "auto" values that depend on a different aligment property that's set on the parent.

SO, I think we need to keep these member-variables "private", and either:
 1) Mark whichever animation class needs access (StyleAnimationValue?) as a firend class, OR
 2) Add custom code to StyleAnimationValue for handling these properties.

I think (1) is probably fine, since StyleAnimationValue is just swapping between values here, so it's OK if it sees values that happen to have a fallback value encoded in the high bits etc.  (It doesn't have to care about the exact representation.)

So, I expect the only change you need in this file is to add "friend class mozilla::StyleAnimationValue", after the existing "friend class nsRuleNode" (which you're currently removing but which should actually stay in).  (Or maybe something other than StyleAnimationValue needs to be a friend class; not sure)

::: layout/style/nsStyleStruct.h:1712
(Diff revision 2)
> +public:
>    uint16_t      mAlignContent;          // [reset] fallback value in the high byte
>    uint8_t       mAlignItems;            // [reset] see nsStyleConsts.h
>    uint8_t       mAlignSelf;             // [reset] see nsStyleConsts.h
> +  uint8_t       mFlexDirection;         // [reset] see nsStyleConsts.h
> +  uint8_t       mFlexWrap;              // [reset] see nsStyleConsts.h

Is there any reason you're moving these member-variables (mFlexDirection/mFlexWrap)? I think they can just stay where they are. (which will mean you don't need to mess with constructor initialization lists further down in the patch.)

(I suspect the "friend" line noted above is literally the only change that this patch needs to make in this file.)
Attachment #8760600 - Flags: review?(dholbert) → review-
(You'll want to update the commit message for the old "Part 3" (the whitespace-removal patch) to be "Part 4", now that you've inserted a new testing patch that's got the label "Part 3".)
(In reply to Daniel Holbert [:dholbert] from comment #21)
> Comment on attachment 8760600 [details]
> Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment
> properties.
> 
> https://reviewboard.mozilla.org/r/58138/#review55482
> 
> ::: layout/style/nsStyleStruct.h:1708
> (Diff revision 2)
> >    mozilla::StyleBoxSizing mBoxSizing;   // [reset] see nsStyleConsts.h
> > -private:
> > -  friend class nsRuleNode;
> >  
> > +public:
> >    uint16_t      mAlignContent;          // [reset] fallback value in the high byte
> 
> Hmm... So on further reflection, I don't think we want to make these members
> public, actually. (That's the main change that you're currently making to
> this file.)
> 
> They're currently "private" for a reason -- most code should only access
> them via the accessors, which take care of masking away "fallback values"
> that are encoded into their high bytes, and which also take care of
> resolving "auto" values that depend on a different aligment property that's
> set on the parent.
> 
> SO, I think we need to keep these member-variables "private", and either:
>  1) Mark whichever animation class needs access (StyleAnimationValue?) as a
> firend class, OR
>  2) Add custom code to StyleAnimationValue for handling these properties.
> 
> I think (1) is probably fine, since StyleAnimationValue is just swapping
> between values here, so it's OK if it sees values that happen to have a
> fallback value encoded in the high bits etc.  (It doesn't have to care about
> the exact representation.)
> 
> So, I expect the only change you need in this file is to add "friend class
> mozilla::StyleAnimationValue", after the existing "friend class nsRuleNode"
> (which you're currently removing but which should actually stay in).  (Or
> maybe something other than StyleAnimationValue needs to be a friend class;
> not sure)
> 
> ::: layout/style/nsStyleStruct.h:1712
> (Diff revision 2)
> > +public:
> >    uint16_t      mAlignContent;          // [reset] fallback value in the high byte
> >    uint8_t       mAlignItems;            // [reset] see nsStyleConsts.h
> >    uint8_t       mAlignSelf;             // [reset] see nsStyleConsts.h
> > +  uint8_t       mFlexDirection;         // [reset] see nsStyleConsts.h
> > +  uint8_t       mFlexWrap;              // [reset] see nsStyleConsts.h
> 
> Is there any reason you're moving these member-variables
> (mFlexDirection/mFlexWrap)? I think they can just stay where they are.
> (which will mean you don't need to mess with constructor initialization
> lists further down in the patch.)
> 
> (I suspect the "friend" line noted above is literally the only change that
> this patch needs to make in this file.)

Thank you for the comment.
I'll do that!
(In reply to Daniel Holbert [:dholbert] from comment #22)
> (You'll want to update the commit message for the old "Part 3" (the
> whitespace-removal patch) to be "Part 4", now that you've inserted a new
> testing patch that's got the label "Part 3".)

Oh, I'm sorry...
Comment on attachment 8760601 [details]
Bug 1277433 - Part 2: Add tests for CSS Alignment.

https://reviewboard.mozilla.org/r/58140/#review55534

I want to have one more look after we add the function to check whether or not a property is supported.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:35
(Diff revision 2)
> +  "justify-self": [
> +    discrete("baseline", "last-baseline")
> +  ],
> +}
> +
> +Object.keys(gCSSProperties).forEach(function(property) {

Can we just use "for (property in gCSSProperties)" here?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:36
(Diff revision 2)
> +    discrete("baseline", "last-baseline")
> +  ],
> +}
> +
> +Object.keys(gCSSProperties).forEach(function(property) {
> +  var testFunctions = gCSSProperties[property];

Before doing anything we need to test that the property is supported by the implementation.

We should add a separate method for this that create an animation (e.g. by creating an element and calling Element.animate), passes in a keyframe object which specifies the property, and then add a getter on that object that checks if the implementation reads the property or not. If the implementation reads the property then we can assume it supports it.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:46
(Diff revision 2)
> +      var keyframe = {};
> +      keyframe[idlName] = [from, to];
> +      var div = createDiv(t);
> +      var animation = div.animate(keyframe, { duration: 1000, fill: "both" });

This is a very minor nit, but it seems like we could just write:

var animation = createDiv(t).animate(
  { idlName: [from, to] },
  { duration: 1000, fill: 'both' });

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:60
(Diff revision 2)
> +      var animation = div.animate(keyframe,
> +                                  { duration: 1000, fill: "both",
> +                                    easing: "cubic-bezier(0.68,0,1,0.01)" });

Can we add a comment explaining what this curve looks like (like this one):

https://dxr.mozilla.org/mozilla-central/rev/f8ad071a6e14331d73fa44c8d3108bc2b66b2174/testing/web-platform/tests/web-animations/animation-model/animation-types/discrete-animation.html#91

Likewise in the next test.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:81
(Diff revision 2)
> +                                           { time: 960,  expected: to }]);
> +    }, "Test " + property + " with keyframe easing");
> +  }
> +}
> +
> +function assertAnimation(idlName, animation, testcases) {

I think we should call this something like:

testAnimationSamples?

And maybe: "animation, idlName, testSamples" for the parameters?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:92
(Diff revision 2)
> +                  " at " + testcase.time + "ms");
> +  });
> +}
> +
> +function propertyToIDL(property) {
> +  if (property == "float") {

Nit: I think Javascript style is to use === whenever possible.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:96
(Diff revision 2)
> +  return property.replace(/-([a-z])/gi, function(str, group) {
> +    return group.toUpperCase();
> +  });

Nit: We could probably drop the use of grouping by just doing:

  replace(/-[a-z]/gi, function (str) { return str.substr(1).toUpperCase(); })
Attachment #8760601 - Flags: review?(bbirtles)
https://reviewboard.mozilla.org/r/58440/#review55558

I'd like to look at this again after it is rebased on the changes to part 2.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:73
(Diff revision 1)
>        var animation = div.animate(keyframe, { duration: 1000, fill: "both" });
>        assertAnimation(idlName, animation, [{ time: 0,    expected: from },
>                                             { time: 499,  expected: from },
>                                             { time: 500,  expected: to },
>                                             { time: 1000, expected: to }]);
> -    }, "Test " + property + " with linear easing");
> +    }, "Test " + property + " <discrete> with linear easing");

Let's make this message something more descriptive like:

property + " uses discrete animation when animating between '" + from "' and '" + to "' with linear easing"

Likewise for the other messages.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:103
(Diff revision 1)
> +      var idlName = propertyToIDL(property);
> +      var keyframe = {};
> +      keyframe[idlName] = ["10px", "50px"];
> +      var div = createDiv(t);

This could use the same sort of simplifications suggested for part 2.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:111
(Diff revision 1)
> +      var div = createDiv(t);
> +      var animation = div.animate(keyframe, { duration: 1000, fill: "both" });
> +      assertAnimation(idlName, animation, [{ time: 0,    expected: "10px" },
> +                                           { time: 500,  expected: "30px" },
> +                                           { time: 1000, expected: "50px" }]);
> +    }, "Test " + property + " <length>");

property + " supports animating as a length"

Likewise for the other messages

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:135
(Diff revision 1)
> +function integer() {
> +  return function(property) {
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var keyframe = {};
> +      keyframe[idlName] = [1, 5];

We should probably test negative values -- or if flex-shrink/order don't support negative values call this positiveInteger or something like that (and then probably apply an easing function to check that we clamp values when we go negative).

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:145
(Diff revision 1)
> +function lengthPercentageCalc() {
> +  return function(property) {
> +    length()(property);
> +    percentage()(property);

What about calc() ?

Also lets call this lengthPercentageOrCalc() to match the naming in the spec.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:154
(Diff revision 1)
> +  if (animation.effect.getKeyframes().length == 0) {
> +    // Ignore if testing property is not supported.
> +    return;
> +  }

We should test for supported properties before running any of the tests on that property.
I'm just realizing that "EnumU8" is incorrect here, because this variable isn't stored in a uint8_t (which is what this "EnumU8" animation flavor is expecting) -- it's stored in a uint16_t!  (See the declaration of mAlignContent in nsStyleStruct.h.)  Same goes for mJustifyContent.

So, I suspect this patch might do something very wrong for these variables...

Also, stepping back a bit -- as dbaron just noted in bug 1064937 comment 7, we'd like to make eStyleAnimType_None "Just Work" for discrete animations, anyway.  That means the s/None/EnumU8/ changes in this patch should be unnecessary, once that bug's resolved. So: we probably shouldn't change nsCSSPropList.h in this patch/bug after all.

We *might* want to still add the "friend" class declaration -- that might still be a necessary prerequisite for bug 1064937.  I'm not entirely sure, though.  Probably worth waiting on that until we've actually got a patch strategy for bug 1064937, and we can add the friend class declaration at that point, if it's needed?

Might still be worth taking this bug's tests, too (I haven't looked at them so I'm not sure) -- but it might also be better if we tested animation of *all* keyword-valued properties (not just this handful) using a more generic mochitest that runs across everything in gCSSProperties and animates between the initial_value and one of the other_values (which are guaranteed to be different and hence would be useful animation endpoints, regardless of the property).
(Sorry, comment 28 was missing some context -- the first paragraph was meant to point to this chunk of patch 1:
> +++ b/layout/style/nsCSSPropList.h
> @@ -333,8 +333,8 @@
>      "",
>      VARIANT_HK,
>      kAutoCompletionAlignJustifyContent,
> -    CSS_PROP_NO_OFFSET,
> -    eStyleAnimType_None)
> +    offsetof(nsStylePosition, mAlignContent),
> +    eStyleAnimType_EnumU8)
(In reply to Daniel Holbert [:dholbert] from comment #28)
> Also, stepping back a bit -- as dbaron just noted in bug 1064937 comment 7,
> we'd like to make eStyleAnimType_None "Just Work" for discrete animations,
> anyway.  That means the s/None/EnumU8/ changes in this patch should be
> unnecessary, once that bug's resolved.

No, we still need eStyleAnimType_None to mark properties that should not be animatable (e.g. animation-duration etc.). We're currently going through all the CSS properties to find out if there are others apart from animation-*/transition-*/display that should not be animatable.
(In reply to Daniel Holbert [:dholbert] from comment #28)
> Might still be worth taking this bug's tests, too (I haven't looked at them
> so I'm not sure) -- but it might also be better if we tested animation of
> *all* keyword-valued properties (not just this handful) using a more generic
> mochitest that runs across everything in gCSSProperties and animates between
> the initial_value and one of the other_values (which are guaranteed to be
> different and hence would be useful animation endpoints, regardless of the
> property).

We're approaching this piecemeal (spec by spec -- hence this bug just covers flexbox/alignment) and setting up the tests in a format we can submit to web-platform-tests. We're updating the specs as we go too. We've done some interop testing with Chrome and there are significant differences for a lots of properties so we're trying to iron them all out in our implementations, in specs, in web-platform-tests, and in Blink too.

Eventually the test added in this file will cover more properties.
MozReview-Commit-ID: DrFjQzSmiI8
Attachment #8763025 - Flags: review?(bbirtles)
MozReview-Commit-ID: 6ozB69Mzy5Z
Attachment #8763026 - Flags: review?(bbirtles)
Comment on attachment 8763025 [details] [diff] [review]
Part 2: Add tests for CSS Alignment

Review of attachment 8763025 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with changes made

::: testing/web-platform/meta/MANIFEST.json
@@ +29047,5 @@
>          "path": "web-animations/animation-model/animation-types/discrete-animation.html",
>          "url": "/web-animations/animation-model/animation-types/discrete-animation.html"
>        },
>        {
> +        "path": "web-animations/animation-model/animation-types/animation-types.html",

It seems like this is not in alphabetical order?

We should use ./mach web-platform-tests --manifest-update to update this file.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Tests for all animation type</title>

Tests for animation types

@@ +1,4 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Tests for all animation type</title>
> +<link rel="help" href="http://w3c.github.io/web-animations/">

https://w3c.github.io/web-animations/#animation-types

@@ +98,5 @@
> +}
> +
> +function isSupported(property) {
> +  var idlName = propertyToIDL(property);
> +  var testKeyframe = new TestKeyframe(idlName);

Nit: We could just write:

  var testKeyframe = new TestKeyframe(propertyToIDL(property));

@@ +123,5 @@
> +function propertyToIDL(property) {
> +  if (property === "float") {
> +    return "cssFloat";
> +  }
> +  // https://drafts.csswg.org/cssom/#css-property-to-idl-attribute

Perhaps we could just add a comment at the start of the function that references: https://w3c.github.io/web-animations/#animation-property-name-to-idl-attribute-name
?
Attachment #8763025 - Flags: review?(bbirtles) → review+
Comment on attachment 8763026 [details] [diff] [review]
Part 3: Add tests for CSS Flexbox

Review of attachment 8763026 [details] [diff] [review]:
-----------------------------------------------------------------

I've been thinking about how we structure these tests and I think we could arrange it like:

animation-types/animation-types-types.html -> animation-types/type-per-property.html
 - Make this test just check that we use the right animation type. As a result we don't need all the different variations for the animation type.

animation-types/length-percentage-or-calc.html
 - Make this just use one representative property and test the more difficult conditions (e.g. em-based units, different variations of calc properties, etc.)

Likewise for other complicated types. That would mean we keep animation-types/discrete-animation.html (perhaps rename to just discrete.html?) and test the different combinations of keyframe/effect easing there.

What do you think?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html
@@ +35,5 @@
> +  ],
> +  "flex-grow": [
> +    // https://drafts.csswg.org/css-flexbox/#flex-grow-property
> +    positiveInteger(),
> +    positiveFloat()

I think this should just be positiveNumber().
(i.e. replace *both* positiveInteger() and positiveFloat() with just positiveNumber())

@@ +40,5 @@
> +  ],
> +  "flex-shrink": [
> +    // https://drafts.csswg.org/css-flexbox/#propdef-flex-shrink
> +    positiveInteger(),
> +    positiveFloat()

Likewise here.

@@ +202,5 @@
> +
> +function float() {
> +  return function(property) {
> +    positiveFloat()(property);
> +    negativeFloat()(property);

As discussed, let's just rename float to number.

@@ +243,5 @@
> +  return function(property) {
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var keyframe = {};
> +      keyframe[idlName] = ["calc(20px - 10px)", "calc(100px - 50px)"];

Rather than this, perhaps we should test with some values that can't be resolved to lengths.

e.g.

calc(100px + 10%) -> calc(10px + 30%)

should give us

calc(55px + 20%)

@@ +276,5 @@
>  function isSupported(property) {
>    var idlName = propertyToIDL(property);
>    var testKeyframe = new TestKeyframe(idlName);
>    try {
> +    // May throw an exception if no keyframe value.

I don't understand this. Why would there be no keyframe value?
Attachment #8763026 - Flags: review?(bbirtles)
I spoke to dbaron about an alternative approach to this where the suggestion was that for these properties that only support discrete animation, we store the animation keyframe values as StyleAnimationValue objects that contain an nsCSSValue. That would potentially save converting back and forth between specified and computed values unnecessarily. David had a patch to do with from several years ago but we need to work out if it still makes sense. After having a quick look, I think it would.

We currently store the keyframe endpoints as nsCSSValues inside KeyframeEffectReadOnly::mKeyframes. Then, at certain times such as when the style context changes we produce computed values (StyleAnimationValue objects) that we store as KeyframeEffectReadOnly::mProperties. When interpolation fails, by implementing the 50% switch behavior, we'll add one of those StyleAnimationValue object representing segment endpoints to the AnimValuesStyleRule. Then, in AnimValuesStyleRule::MapRuleInfoInto we'll call StyleAnimationValue::UncomputeValue to get back an nsCSSValue from the StyleAnimationValue object. Presumably if we store the value as an nsCSSValue all along we'd save some work here. That would also mean we don't need to make the various mFlexDirection etc. members public and we could leave the declarations in nsCSSPropList.h as CSS_PROP_NO_OFFSET.

Dvaid, does that sound right?
Flags: needinfo?(dbaron)
I think so.  Though I'm not quite sure how all the details will work out.
Flags: needinfo?(dbaron)
(In reply to Brian Birtles (:birtles) from comment #35)
> @@ +276,5 @@
> >  function isSupported(property) {
> >    var idlName = propertyToIDL(property);
> >    var testKeyframe = new TestKeyframe(idlName);
> >    try {
> > +    // May throw an exception if no keyframe value.
> 
> I don't understand this. Why would there be no keyframe value?

As discussed, I think this should be something like:

"Since TestKeyframe returns 'undefined' for |property|, the KeyframeEffect constructor will throw if this does not produce a valid keyframe value."

Hi Daniel,
Could you advise to me?

I want to set nsCSSValue into StyleAnimationValue at StyleAnimationValue::ExtractComputedValue for the properties that can animate as discrete animation.
http://searchfox.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp#3590

This is the code I wrote;

    case eStyleAnimType_Discrete: { // I appended eStyleAnimType_Discrete to nsCSSPropList.h
      int indexInStruct = nsCSSProps::PropertyIndexInStruct(aProperty);
      void* dataStorage = alloca((indexInStruct + 1) * sizeof(nsCSSValue));
      nsCSSValue* cssValueStorage = static_cast<nsCSSValue*>(dataStorage);
      new (cssValueStorage + indexInStruct) nsCSSValue();
      nsStyleStructID sid = nsCSSProps::kSIDTable[aProperty];
      nsRuleData ruleData(nsCachedStyleData::GetBitForSID(sid),
                          cssValueStorage, aStyleContext->PresContext(),
                          aStyleContext);
      ruleData.mValueOffsets[sid] = 0;
      aStyleContext->RuleNode()->GetRule()->MapRuleInfoInto(&ruleData);

      nsCSSValue* value = ruleData.ValueFor(aProperty);
      nsCSSValue* copyValue = new nsCSSValue(*value);
      aComputedValue.SetAndAdoptCSSValueValue(copyValue, eUnit_CSSValue); // I appended also eUnit_CSSValue.
      return true;
    }

Actually, this code retrieves correct nsCSSValue. However, it does not look optimal way.
So, May I ask two questions?

1. Is it possible to get the nsCSSValue that already generated (e.g. KeyframeEffect.mKeyframes) from here?
2. If 1 is impossible, I’d like to know the best way to retrieve nsCSSValue from nsStyleContext.

Thanks!
I'll continue to investigate too.
Flags: needinfo?(dholbert)
I'm sorry.
Couldn't you give me an advice about only second question(how to retrieve nsCSSValue from nsStyleContext)?
Are you sure you want a a nsCSSValue here? nsStyleContext doesn't have access to any nsCSSValues. nsCSSValue is a catch-all object which generally stores "specified style" values, produced by the CSS Parser.  In contrast, the data that's available via nsStyleContext* is "computed style" values, which are a bit more processed/resolved down to specific types -- e.g. uint8_t or uint16_t for enum-valued types, like the flexbox properties discussed in this bug.

You shouldn't necessarily need a nsCSSValue (or to use SetAndAdoptCSSValueValue). You could just grab uint8_t (or uint16_t) for the property in question off of its style struct, and call "aComputedValue.SetIntValue([whatever], eUnit_Enumerated);" We already do exactly that for eStyleAnimType_EnumU8, actually.

But maybe you're wanting something that can populate the StyleAnimationValue (aComputedValue) *regardless* of the representation in the style struct? (whether it's uint8_t, uint16_t, or something more complicated)  I don't think we have any general-purpose code to do that (via a nsCSSValue intermediate or otherwise).
Flags: needinfo?(dholbert)
In other/briefer words: "retrieve nsCSSValue from nsStyleContext" isn't something that's really possible.

(We do *generate* one-off nsCSSValues in StyleAnimationValue as a convenient storage mechanism, for *certain specific* properties/intermediate-representations.  But, there's no generic code/function to produce a nsCSSValue for any arbitrary property, based on its style struct / nsStyleContext.)

I don't know what the current plan is here, but one option I can imagine (and perhaps what birtles had in mind in comment 36 RE "store the value as an nsCSSValue all along") would be: make the StyleAnimation code (or its clients) *completely bypass* ExtractComputedValue for this discrete-animation scenario, and instead create a CSS Parser to directly produce a nsCSSValue for the given property, via e.g. nsCSSParser::ParseLonghandProperty().  I'm not sure, though.
(In reply to Daniel Holbert [:dholbert] from comment #42)
> I don't know what the current plan is here, but one option I can imagine
> (and perhaps what birtles had in mind in comment 36 RE "store the value as
> an nsCSSValue all along") would be: make the StyleAnimation code (or its
> clients) *completely bypass* ExtractComputedValue for this
> discrete-animation scenario, and instead create a CSS Parser to directly
> produce a nsCSSValue for the given property, via e.g.
> nsCSSParser::ParseLonghandProperty().  I'm not sure, though.

Yeah, the tricky part is when we have to fill in a value using the current style. For example, a CSS animation missing a 0% keyframe. In that case the spec says to use the underlying style (specifically, "If a 0% or from keyframe is not specified, then the user agent constructs a 0% keyframe using the computed values of the properties being animated."). Currently we do that by generating a style context without animation rules applied and then we extract an nsCSSValue from that by using StyleAnimationValue::ExtractComputedValue (to get a StyleAnimationValue object representing the computed value) and then passing that to StyleAnimationValue::UncomputeValue (to turn it into an nsCSSValue). (This happens in CSSAnimationBuilder::GetComputedValue.)

I think the hope here was to find a generic way to do this for any types that we don't currently handle in StyleAnimationValue like you mentioned in comment 41. It sounds like that might not be possible, however.
At the end of the process, we need an nsCSSValue to put into nsRuleData. We currently do this in AnimValuesStyleRule where we store the values to be added as StyleAnimationValue objects and then convert them to nsCSSValue objects by calling StyleAnimationValue::UncomputeValue.

There is certainly a lot of back and forth between nsCSSValue objects and StyleAnimationValue objects (especially for CSS Animations and CSS Transitions where we initially create a StyleAnimationValue object then convert it to an nsCSSValue only to convert it back to an StyleAnimationValue object again--a lot of which comes down to limitations introduced by handling CSS variables correctly).

One way of handling discrete animation types is to add an appropriate type for each property that we want to be able to animate discretely, e.g. eStyleAnimType_EnumU16. However, there's a fair bit of work involved there since we have all sorts of types that should use discrete animation but which are enumerated types such as, e.g., background-repeat.

I like the idea of standing up an nsCSSParser for this. That will work fine for properties specified using the API but I wonder what we will do for CSS Animations/CSS Transitions where we only have an nsStyleContext from which to pull the different values? I wonder if we could re-use the machinery in nsComputedDOMStyle::GetPropertyCSSValue for this? Daniel, any ideas?
Flags: needinfo?(dholbert)
(Sorry for the delayed response here -- with the combination of post-vacation-backlog + helping some interns start out last week + a US holiday this week, I'm a bit behind on replies. :))

(In reply to Brian Birtles (:birtles) from comment #44)
> There is certainly a lot of back and forth between nsCSSValue objects and
> StyleAnimationValue objects (especially for CSS Animations and CSS
> Transitions where we initially create a StyleAnimationValue object then
> convert it to an nsCSSValue only to convert it back to an
> StyleAnimationValue object again--a lot of which comes down to limitations
> introduced by handling CSS variables correctly).

(Side note: the CSS-variable-handling animation code will be getting more complex soon -- Jonathan Chan (an intern working out of SF) is working on supporting author-provided type information for CSS Variables (bug 1273706), which will mean we'll be able to set up animations for CSS Variables themselves.)

> One way of handling discrete animation types is to add an appropriate type
> for each property that we want to be able to animate discretely, e.g.
> eStyleAnimType_EnumU16. However, there's a fair bit of work involved there
> since we have all sorts of types that should use discrete animation but
> which are enumerated types such as, e.g., background-repeat.

Yeah... I expect we'd be able to handle most cases with EnumU8/16-type stuff, but there would definitely be a pile of properties that would require special handling.

> I like the idea of standing up an nsCSSParser for this. That will work fine
> for properties specified using the API but I wonder what we will do for CSS
> Animations/CSS Transitions where we only have an nsStyleContext from which
> to pull the different values? I wonder if we could re-use the machinery in
> nsComputedDOMStyle::GetPropertyCSSValue for this? Daniel, any ideas?

Good idea! It looks like we already do this for the SMIL function "GetCSSComputedValue", which returns a stringified computed-style representation (by reference), using ComputedDOMStyle machinery:
http://searchfox.org/mozilla-central/source/dom/smil/nsSMILCSSProperty.cpp#22
Flags: needinfo?(dholbert)
Daisuke, did you mean to re-request review here? I got four pieces of bugzilla-mail ~12 hours ago which said review had been requested:
> review requested: [Bug 1277433] Use discrete animation for appropriate flexbox properties
> [Attachment 8760600 [details]] Bug 1277433  Part 1: Use discrete animation for appropriate CSS Alignment properties.
...but Bugzilla and MozReview don't seem to show any open review requests here, so I think it may have just been a MozReview<-->Bugzilla-Mail bug.

For now, I'm assuming there are no open review requests here, but let me know if I missed something. :)
Hi Daniel!
Oh, I actually pushed to MozReview, but could not publish from the web site since I got an error message that Birtles can not review today. I think, the mails were sent at that time... sorry,,
Of course, I could upload the patch as an attachment to this bug, but I canceled since I'd like to investigate about how to get nsComputedDOMStyle a little more.

I paste the current code, anyway.

    case eStyleAnimType_Discrete: {
      nsIStyleRule *rule = aStyleContext->RuleNode()->GetRule();
      RefPtr<css::Declaration> declaration(do_QueryObject(rule));
      if (declaration) {
        nsAutoString value;
        declaration->GetValue(aProperty, value);
        nsIDocument* doc =
          aStyleContext->PresContext()->GetPresShell()->GetDocument();
        nsCSSParser  parser(doc->CSSLoader());
        nsCSSValue   cssValue;
        parser.ParseLonghandProperty(aProperty,
                                     value,
                                     doc->GetDocumentURI(),
                                     doc->GetDocumentURI(),
                                     doc->NodePrincipal(),
                                     cssValue);
        nsCSSValue* copiedCSSValue = new nsCSSValue(cssValue);
        aComputedValue.SetAndAdoptCSSValueValue(copiedCSSValue, eUnit_CSSValue);
        return true;
      }
      return false;
    }
Nits:
 (1) "nsIStyleRule *" -- shift "*" to the left, to be alongside the type.
 (2) Probably drop extra space characters in "nsCSSParser  parser" and "nsCSSValue   cssValue"
 (3) GetPresShell() and GetDocument() are allowed to return null (that's what the "get" indicates), so you need to null-check them & allow for that possibility, probably with a null-check-and-early-return.
 (4) For consistency with those new null-checks-and-early-returns, you might want to flip the logic of your "if (declaration)" to be "if (!declaration) { return false; }" and then deindent everything after that.
 (5) We're shifting from "new" to MakeUnique for singly-owned objects, and avoiding storing such things in raw pointers, where possible. So: please declare copiedCSSValue as:
    auto copiedCSSValue = MakeUnique<nsCSSValue>(cssValue);
    aComputedValue.SetAndAdoptCSSValueValue(copiedCSSValue.release(), eUnit_CSSValue);
 (Ultimately we should make SetAndAdoptCSSValueValue accept a UniquePtr<> arg, but for now we'll shift out of UniquePtr at its API boundary.)
Wao! Thank you very much!
Attached patch flexbox-1277433-1.patch (obsolete) — — Splinter Review 
Daniel, please review this patch!
I could not use ComputedDOMStyle machinery since that looks getting the styles that are already applied to Element. 
(If I am misunderstanding about ComputedDOMStyle machinery, please let me know.)
Attachment #8772267 - Flags: review?(dholbert)
Attached patch flexbox-1277433-1.patch (obsolete) — — Splinter Review 
I replaced the patch.
Attachment #8760600 - Attachment is obsolete: true
Attachment #8772267 - Attachment is obsolete: true
Attachment #8772267 - Flags: review?(dholbert)
Attachment #8772312 - Flags: review?(dholbert)
Comment on attachment 8772312 [details] [diff] [review]
flexbox-1277433-1.patch

Review of attachment 8772312 [details] [diff] [review]:
-----------------------------------------------------------------

High-level comment -- we probably should try to cover as many properties as possible with this change (or before shipping this change, at least).  As dbaron said in bug 1064937 comment 7:
> I'd rather not expose that somewhat-random set of properties; I'd prefer to make this change all at once.

birtles presented a counterpoint to this in comment 31, but until I've heard otherwise from dbaron, I should defer to his judgement on this (and I agree that it'd be bad if we shipped this in a bit-by-bit sort of way; we should try to fix as much of this at once as possible).

So, I'm hesitant to r+ this until we've evaluated which properties need this sort of a change, & we have a plan for how to make that change across all/most of them. (maybe punting on a few special cases like e.g. shorthands if those need to be special, but covering as much as we can)  They don't need to all change in this bug necessarily, but we should be prepared to change most/all at the same time in a family of related bugs, if possible.

(I expect most of the needed new code will just be additional eCSSUnit_None --> eCSSUnit_Discrete changes.)

::: layout/style/StyleAnimationValue.cpp
@@ +3602,1 @@
>               "must be dealing with animatable property");

The text of this assert needs updating. It already wasn't very clear, and now it's even less clear about what it's asserting.

These days, this message really wants to say something like:
"We can only extract a value for this property if we know its style-struct offset, or we have custom code for this property, or if the property is animated discretely & doesn't need to check its style-struct."

@@ +4226,5 @@
>        return true;
>      }
> +    case eStyleAnimType_Discrete: {
> +      nsIPresShell* shell = aStyleContext->PresContext()->GetPresShell();
> +      if (shell == nullptr) {

This should be: "if (!shell) {"

Per coding style guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices
"When testing a pointer, use (!myPtr) or (myPtr); don't use myPtr != nullptr or myPtr == nullptr"

@@ +4230,5 @@
> +      if (shell == nullptr) {
> +        return false;
> +      }
> +      nsIDocument* doc = shell->GetDocument();
> +      if (doc == nullptr) {

if (!doc) {

@@ +4241,5 @@
> +      }
> +      nsAutoString value;
> +      declaration->GetValue(aProperty, value);
> +      nsCSSValue cssValue;
> +      nsCSSParser parser(doc->CSSLoader());

This seems reasonable (grabbing the value from the declaration from the rule node), but I'm not familiar enough with this code to be sure that there isn't a better way. I'd prefer that dbaron or heycam sanity-check that this makes sense.

@@ +4358,5 @@
>        break;
>      case eUnit_Calc:
>      case eUnit_ObjectPosition:
>      case eUnit_URL:
> +    case eUnit_CSSValue:

These units describe what the thing is & how it's interpolated. (As a color, as an object-position, as a calc expression, etc.)

So: "eUnit_CSSValue" is too vague -- it's not clear how that's interpolated. So, this should be named something more like "eUnit_DiscreteCSSValue", to make that clearer.  (With that, your e.g. new AddWeighted case -- which just returns false -- makes more obvious sense.)
Attachment #8772312 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #54)
> High-level comment -- we probably should try to cover as many properties as
> possible with this change (or before shipping this change, at least).  As
> dbaron said in bug 1064937 comment 7:
> > I'd rather not expose that somewhat-random set of properties; I'd prefer to make this change all at once.
> 
> birtles presented a counterpoint to this in comment 31, but until I've heard
> otherwise from dbaron, I should defer to his judgement on this (and I agree
> that it'd be bad if we shipped this in a bit-by-bit sort of way; we should
> try to fix as much of this at once as possible).

The trouble is we're already shipping this piecemeal. That is, all the properties currently marked as having animation type eStyleAnimType_EnumU8 already exhibit this behavior.

We started changing the other properties in one big hit but we discovered that going through all the CSS properties, working out which ones should use discrete interpolation (and which ones should not be animatable etc.), editing all the specs accordingly, and then finally making up the patch was taking too long. It seemed like we'd make better progress (and avoid the danger of neglecting the spec work) by approaching this spec by spec: edit the spec, update Gecko, then iterate. The spec changes are going to happen spec-by-spec anyway since I want to give spec editors a chance to review the PR for their particular spec.

What do you think?

(You can see some of the work we started doing on this at: http://dadaa.github.io/IsAnimatableCSS/)

> So, I'm hesitant to r+ this until we've evaluated which properties need this
> sort of a change, & we have a plan for how to make that change across
> all/most of them. (maybe punting on a few special cases like e.g. shorthands
> if those need to be special, but covering as much as we can)  They don't
> need to all change in this bug necessarily, but we should be prepared to
> change most/all at the same time in a family of related bugs, if possible.

Once all the review comments have been address can we at least r+ this patch and not land it until the other ones are ready?
(In reply to Brian Birtles (:birtles; away 14-18 July, busy 19-22 July) from comment #55)
> The trouble is we're already shipping this piecemeal. That is, all the
> properties currently marked as having animation type eStyleAnimType_EnumU8
> already exhibit this behavior.

Well, it looks like we only use eStyleAnimType_EnumU8 for SVG-related properties (for historical SMIL purposes) and a few random non-SVG properties right now (for didn't-know-any-better purposes, I think).

So, the current discrete-animation support, while piecemeal, could be considered largely to be one-off exceptions.  And it might be better to expand those exceptions all at once, rather than bit-by-bit.  (I'm not sure; this is really dbaron's call to make, given that he's the module owner & he's expressed concerns about doing this bit-by-bit.)

> The spec changes are going to happen
> spec-by-spec anyway since I want to give spec editors a chance to review the
> PR for their particular spec.

But we are expecting the vast majority of currently-"non-animatable" properties to change like this, though, right? I'd expect the truly-non-animatable properties to be pretty rare.

> Once all the review comments have been address can we at least r+ this patch
> and not land it until the other ones are ready?

I have two concerns right now:

 (1) I don't think the current approach will support animation of shorthand properties (since the patch depends on "ParseLonghandProperty" right now).  Maybe that's OK for the time being, but we do need to come up with some way of supporting discrete animation of shorthands, right? (That doesn't really prevent r+ on this part, I suppose, but it's something we need to think through.)

 (2) As noted in my second-to-last chunk in comment 54, I'd like dbaron/heycam to sanity-check the rule/declaration/parser usage (everything leading up to the new ParseLonghandProperty call).

So, I'm OK r+'ing a patch that has review feedback in comment 54 addressed, given that it also gets r+ from dbaron/heycam on the new case statement. (And I'd defer to dbaron on when we should land it & how much we should care about doing this all at once.)
--> updating bug summary to mention "css alignment properties" instead of "css flexbox properties", per comment 8.
(The commit message was fixed a while back, but the bug summary still has a stale mention of flexbox.)
Summary: Use discrete animation for appropriate flexbox properties → Use discrete animation for appropriate css alignment properties
(In reply to Daniel Holbert [:dholbert] from comment #57)
> > The spec changes are going to happen
> > spec-by-spec anyway since I want to give spec editors a chance to review the
> > PR for their particular spec.
> 
> But we are expecting the vast majority of currently-"non-animatable"
> properties to change like this, though, right? I'd expect the
> truly-non-animatable properties to be pretty rare.

I hope so. But I was feeling unsure about that conclusion when I remembered that in SVG, despite the fact that almost all attributes are animatable in a discrete manner, certain attributes are marked as non-animatable, e.g. some bidi-related ones. I wasn't 100% sure that there were no similar cases in CSS where we explicitly want to avoid allowing animation so I thought I should probably go and check--and that was taking a long time so I was hoping to tackle this spec-by-spec in order to unblock Daisuke and avoid spec editing fatigue. Maybe it's fine to just assume it's only display, transition-*, and animation-* that we need to mark as non-animatable though.

> > Once all the review comments have been address can we at least r+ this patch
> > and not land it until the other ones are ready?
> 
> I have two concerns right now:
> 
>  (1) I don't think the current approach will support animation of shorthand
> properties (since the patch depends on "ParseLonghandProperty" right now). 
> Maybe that's OK for the time being, but we do need to come up with some way
> of supporting discrete animation of shorthands, right? (That doesn't really
> prevent r+ on this part, I suppose, but it's something we need to think
> through.)

This should be fine. We don't animate shorthands -- we expand them to longhands and animate the components (hence shorthands don't have an animation type).

>  (2) As noted in my second-to-last chunk in comment 54, I'd like
> dbaron/heycam to sanity-check the rule/declaration/parser usage (everything
> leading up to the new ParseLonghandProperty call).

Absolutely. Daisuke will get heycam to have a look since I believe dbaron is moving around a lot presently.
(In reply to Brian Birtles (:birtles) from comment #59)
> I hope so. But I was feeling unsure about that conclusion when I remembered
> that in SVG, despite the fact that almost all attributes are animatable in a
> discrete manner, certain attributes are marked as non-animatable, e.g. some
> bidi-related ones.

I don't think it makes any sense for SVG to have non-animatable properties.  (This seems impossible anyway given that the bidi-related properties aren't SVG specific and that there'd be no way to specialise the interpolation behaviour depending on whether you're using them in SVG or HTML content.  All properties shared with CSS should now be defined by reference in SVG 2 to CSS specs.)  So I would proceed assuming that any "Animatable: no" lines for properties defined in SVG mean the same as in CSS, i.e. discrete animation.
Comment on attachment 8760600 [details]
Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58138/diff/2-3/
Attachment #8760600 - Attachment is obsolete: false
Attachment #8760600 - Flags: review?(dholbert)
Attachment #8760600 - Flags: review?(cam)
Attachment #8760600 - Flags: review-
Attachment #8760601 - Flags: review?(bbirtles)
Attachment #8761083 - Flags: review?(bbirtles)
Comment on attachment 8760601 [details]
Bug 1277433 - Part 2: Add tests for CSS Alignment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58140/diff/2-3/
Comment on attachment 8761083 [details]
Bug 1277433 - Part 3: Add tests for CSS Flexbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58440/diff/1-2/
Attachment #8760602 - Attachment is obsolete: true
Attachment #8772312 - Attachment is obsolete: true
Attachment #8763026 - Attachment is obsolete: true
Hello Cameron,

As Daniel suggested in comment 55, could you review the patch? Especially how to grab the value from nsStyleContext.

Thanks.
Comment on attachment 8760600 [details]
Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.

https://reviewboard.mozilla.org/r/58138/#review63938

::: layout/style/StyleAnimationValue.h:290
(Diff revision 3)
>      eUnit_Shape,  // nsCSSValue::Array* (never null)
>      eUnit_Filter, // nsCSSValueList* (may be null)
>      eUnit_Transform, // nsCSSValueList* (never null)
>      eUnit_BackgroundPositionCoord, // nsCSSValueList* (never null)
>      eUnit_CSSValuePairList, // nsCSSValuePairList* (never null)
> +    eUnit_DiscreteCSSValue, // nsCSSValue* (never null)

Let's put this up with the other types that take an nsCSSValue (just below eUnit_URL).

::: layout/style/StyleAnimationValue.cpp:3599
(Diff revision 3)
> +             "should extract the value from style-struct or custom code, " \
> +             "otherwise the animation type is discrete");

If I can suggest a clearer way to word the assertion message: "all animation types other than Custom and Discrete must specify a style struct offset to extract values from".

::: layout/style/StyleAnimationValue.cpp:4236
(Diff revision 3)
> +      nsIStyleRule* rule = aStyleContext->RuleNode()->GetRule();
> +      RefPtr<css::Declaration> declaration(do_QueryObject(rule));
> +      if (!declaration) {
> +        return false;
> +      }
> +      nsAutoString value;
> +      declaration->GetValue(aProperty, value);
> +      nsCSSValue cssValue;
> +      nsCSSParser parser(doc->CSSLoader());
> +      parser.ParseLonghandProperty(aProperty,
> +                                   value,
> +                                   doc->GetDocumentURI(),
> +                                   doc->GetDocumentURI(),
> +                                   doc->NodePrincipal(),
> +                                   cssValue);

Why do we need to go through the nsCSSParser with the serialization of the property value we get from the Declaration, rather than just copy it?  (Does it reset the base URI or something?)

Also, the Declaration you get from nsRuleNode will only have values for the most specific rule that matched the element.  For example if we had:

  <style>
  div { font-style: italic; }
  div.a { color: blue; }
  </style>
  <div class=a>...</div>

and we want to extract font-style for discrete animation, we won't find it on the rule node's declaration -- that only has color.  So you would need to walk up the rule tree to find the first one that has aProperty.

But we also have nsIStyleRule implementations that aren't css::Declaration objects, such as those that map HTML presentation attributes into style (like HTMLColorRule, http://searchfox.org/mozilla-central/rev/336105a0de49f41a2cc203db7b7ee7266d0d558b/layout/style/nsHTMLStyleSheet.cpp#45) but also ImportantStyleData (http://searchfox.org/mozilla-central/source/layout/style/Declaration.h#52).

The nsIStyleRule interface is pretty limited.  You could add a new method to it to get the value of a specific nsCSSProperty, although that would be a bit of work.  You could also just use the existing MapRuleInfoInto method to fill in an nsRuleData, walking up the rule tree, until the property you are interested in has been filled in.  This might be a bit wasteful, though.  I would try the former approach, if there's no other way to get the nsCSSValue you need.
Attachment #8760600 - Flags: review?(cam) → review-
Attachment #8760600 - Flags: review?(dholbert) → review+
Comment on attachment 8760600 [details]
Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.

https://reviewboard.mozilla.org/r/58138/#review64202

r=me on everything but the rulenode/parser stuff that heycam brought up (modulo heycam's nits - I agree that the new enum should be declared up next to the other nsCSSValue-flavored enums)

Perhaps best to have heycam review any new style-system stuff you add here (to address his concerns about rulenode/parser/etc.), and feel free to carry forward r=dholbert on the rest of the patch.
Comment on attachment 8760601 [details]
Bug 1277433 - Part 2: Add tests for CSS Alignment.

https://reviewboard.mozilla.org/r/58140/#review64386

::: testing/web-platform/tests/web-animations/animation-model/animation-types/animation-types.html:128
(Diff revision 3)
> +  // https://w3c.github.io/web-animations/#animation-property-name-to-idl-attribute-name
> +  if (property === "float") {
> +    return "cssFloat";
> +  }
> +  return property.replace(/-[a-z]/gi,
> +                          function (str) { return str.substr(1).toUpperCase(); });

Nit: This is slightly over 80 characters
Attachment #8760601 - Flags: review?(bbirtles) → review+
Comment on attachment 8761083 [details]
Bug 1277433 - Part 3: Add tests for CSS Flexbox.

https://reviewboard.mozilla.org/r/58440/#review64392

This is good but I'd like to check the extra length and calc tests once more.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:126
(Diff revision 2)
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var keyframe = {};
> +      keyframe[idlName] = ["10px", "50px"];
> +      var div = createDiv(t);
> +      var animation = div.animate(keyframe, { duration: 1000, fill: "both" });
> +      testAnimationSamples(animation, idlName,
> +                           [{ time: 0,    expected: "10px" },
> +                            { time: 500,  expected: "30px" },
> +                            { time: 1000, expected: "50px" }]);
> +    }, property + " supports animating as a length");

Could we test using other length units like rem units too?

(We'll probably need to set the font-size on the root element to make sure we know what px value to expect.)

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:158
(Diff revision 2)
> +    positiveInteger()(property);
> +    negativeInteger()(property);

Any reason to test like this? Wouldn't it be valid to test interpolating from negative to positive too?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:196
(Diff revision 2)
> +    positiveNumber()(property);
> +    negativeNumber()(property);

Any reason to test like this? Wouldn't it be valid to test interpolating from negative to positive too?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:253
(Diff revision 2)
> +
> +function lengthPercentageOrCalc() {
> +  return function(property) {
> +    length()(property);
> +    percentage()(property);
> +    calc()(property);

I think we need to test the combinations too?

e.g. animating from 10px -> 20% should give you calc(10px + 0%) -> calc(0px + 20%) which, at the mid-way point would give you calc(5px + 10%) ?

See https://drafts.csswg.org/css-transitions/#animtype-lpcalc

We could test:
* 10px -> 20%
* 30% -> 10em
* -10px -> calc(20px + 30%)
* calc(10px + 20% + 30px) -> 50px

(I'm actually not sure how all of these work)

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:272
(Diff revision 2)
>  function isSupported(property) {
>    var testKeyframe = new TestKeyframe(propertyToIDL(property));
>    try {
> -    // May throw an exception if the keyframe value is not correct.
> -    new KeyframeEffectReadOnly(null, testKeyframe);
> +    // Since TestKeyframe returns 'undefined' for |property|,
> +    // the KeyframeEffect constructor will throw
> +    // if this does not produce a valid keyframe value.

... if the string "undefined" is not a valid value for the property.
Hello Cameron,
I am sorry to bother you while you are busy.

For now, I’m trying to implement adding a method in nsIStyleRule that returns nsCSSValue.
Actually, I could grab the values that are specified by class, style attribute or keyframes. And when there are no specific style, I’d like to use the initial value, but I could not get from the nsIStyleRule and nsRuleNode.

So, I tried to grab the initial value from Element. However, I had to add the Element as a parameter to ExtractComputedValue since there are no ways to access the Element from inside of ExtractComputedValue. And if I modify it, have to modify many methods that call ExtractComputedValue in nsStyleContext( e.g. ExtractColor https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp#1413 ) as well, Also much more other callers call that nsStyleContext's methods, I imagine.
I thought this implementation is not good way,,

So, I have a request regarding that.
Could you advise the better way to get the initial value using nsCSSProperty?

Thanks!
Flags: needinfo?(cam)
Can you just use an nsCSSValue with unit = eCSSUnit_Initial?


Initial values are only available in computed value form, not in specified value form.  These values are available on the root nsRuleNode (they are set in nsRuleNode::SetDefaultOnRoot, when a request for a struct is made), but there is no corresponding nsIStyleRule that sets them.  (Although mRule on the root nsRuleNode is null, you can think of it as having a rule that has |all: initial;|.)

There are only two places in C++ code where initial values for properties can be found, but they are both computed values.  One is in the style struct constructors, e.g. in the nsStyleText(StyleStructContext) constructor, mTextAlign is initialized to NS_STYLE_TEXT_ALIGN_START.  The second is in the nsRuleNode::ComputeXXXData function, e.g. in ComputeTextData the initial value NS_STYLE_TEXT_ALIGN_START is passed to the SetValue() function.

So if you really need specified initial values, and not just eCSSUnit_Initial, they will need to be added somewhere else.  But note that some properties have initial values which depend on the pres context, e.g. default font-family and font-size.  So I don't think you can add the values themselves to a new field in nsCSSPropList.h, and instead you would need a function that can return them.
Flags: needinfo?(cam)
Cameron, thank you very much!

I tried to use eCSSUnit_Initial, but the value of getKeyFrames() of CSS animation had not been computed (e.g. initial value of align-content is "normal" ).
https://dxr.mozilla.org/mozilla-central/source/dom/webidl/KeyframeEffect.webidl#45
I confirmed the spec to Brian a short time ago, we can use uncomputed values for now.
So I'll use eCSSUnit_Initial in this time.

Thanks!
Review commit: https://reviewboard.mozilla.org/r/69210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69210/
Attachment #8777708 - Flags: review?(cam)
Attachment #8777709 - Flags: review?(cam)
Attachment #8777710 - Flags: review?(bbirtles)
Attachment #8777711 - Flags: review?(cam)
Attachment #8760600 - Flags: review- → review?(cam)
Attachment #8761083 - Flags: review?(bbirtles)
Comment on attachment 8760600 [details]
Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58138/diff/3-4/
Comment on attachment 8760601 [details]
Bug 1277433 - Part 2: Add tests for CSS Alignment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58140/diff/3-4/
Comment on attachment 8761083 [details]
Bug 1277433 - Part 3: Add tests for CSS Flexbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58440/diff/2-3/
Comment on attachment 8761083 [details]
Bug 1277433 - Part 3: Add tests for CSS Flexbox.

https://reviewboard.mozilla.org/r/58440/#review66586

r=me with the following change made

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:204
(Diff revision 3)
> +function calc() {
> +  return function(property) {
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var keyframe = {};
> +      keyframe[idlName] = ["calc(10px + 10%)", "calc(50px + 50%)"];
> +      var div = createDiv(t);
> +      var animation = div.animate(keyframe, { duration: 1000, fill: "both" });
> +      testAnimationSamples(animation, idlName,
> +                           [{ time: 0,    expected: "calc(10px + 10%)" },
> +                            { time: 500,  expected: "calc(30px + 30%)" },
> +                            { time: 1000, expected: "calc(50px + 50%)" }]);
> +    }, property + " supports animating as a calc");
> +  }
> +}
> +

I think this can be rolled into lengthPercentageOrCalc (or dropped if it overlaps). I don't think we ever test calc() by itself and this test requires the properties supports lengths and percentages anyway.
Attachment #8761083 - Flags: review?(bbirtles) → review+
Comment on attachment 8777710 [details]
Bug 1277433 - Part 6: Add tests that were eStyleAnimType_EnumU8.

https://reviewboard.mozilla.org/r/69214/#review66588

This is good and getting really close but I think we can make this more simple. I think this patch will become more simple once we remove testAnimation so I'd like to check it again after that.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:22
(Diff revision 1)
>  "use strict";
>  
>  var gCSSProperties = {
> -  "align-content": [
> +  "align-content": {
>      // https://drafts.csswg.org/css-align/#propdef-align-content
> +    testcases: [

s/testscases/tests/ ?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:158
(Diff revision 1)
> +  "ruby-position": {
> +    // https://drafts.csswg.org/css-ruby-1/#propdef-ruby-position
> +    testcases: [
> +      discrete("under", "over")
> +    ],
> +    element: "<ruby id='target'></ruby>"

Can we avoid using innerHTML?

We could either:

a) Create a ruby/svg in the document markup and just use it everytime (since we're always animating different properties I don't think the tests would interfere with each other)

or

b) Just pass in the element name.

Using innerHTML isn't terrible for a test case (although it has security reasons for not using in other contexts) but something about it seems a bit off.

See my comments further below, but I think we can just do (b) and probably rename 'element' here to 'tagName'.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:209
(Diff revision 1)
> +  "vector-effect": {
> +    // https://svgwg.org/svg2-draft/coords.html#VectorEffectProperty
> +    testcases: [
> +      discrete("non-scaling-stroke", "non-scaling-size")
> +    ]
> +  },

Can we just interpolate between 'none' and 'non-scaling-stroke' ?

I'm not sure that we or any of the other browsers are planning on implementing the other values in the near future.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:239
(Diff revision 1)
>  
>  for (var property in gCSSProperties) {
>    if (!isSupported(property)) {
>      continue;
>    }
> -  var testFunctions = gCSSProperties[property];
> +  var task = gCSSProperties[property];

s/task/testData/ ?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:253
(Diff revision 1)
> -      testAnimationSamples(animation, idlName, [{ time: 0,    expected: from },
> +      var testSamples = [{ time: 0,    expected: from },
> -                                                { time: 499,  expected: from },
> +                         { time: 499,  expected: from },
> -                                                { time: 500,  expected: to },
> +                         { time: 500,  expected: to },
> -                                                { time: 1000, expected: to }]);
> +                         { time: 1000, expected: to }];
> +      testAnimation(t, property, options, keyframes, timing, testSamples);

Oh, I prefer how you had it!

Having to write out separate lines for 'timing' and 'testSamples' seems a shame: it's more to write and less clear to read.

Can we
1. In testcommon.js, rename createDiv into createElement and make it take a tagName argument. 
2. In testcommon.js, re-add createDiv and make it call createElement passing 'div' as the tagName.
3. Possibly add a wrapper like so:
  function createTestElement(test, tag) {
    createElement(t, tag || 'div', document);
  }
4. In these tests write:
  var elem = createTestElement(t, options.tagName);

That way we also get the free clean-up behavior that createDiv currently provides (I think the arrangement in this patch doesn't remove the element?)

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:451
(Diff revision 1)
> +      var testSamples = [{ time: 0,    expected: from },
> +                         { time: 1,    expected: to },
> +                         { time: 1000, expected: to }];

I know I said we could do deeper per-type testing in a separate file, but I think maybe we can just do it here.

For 'visibility' I think we should:
* Not pass in the values but just specify them here.
* Test the following combinations
  visibility -> hidden
  hidden -> visibility
  hidden -> collapse
  visibility -> hidden with a cubic-bezier curve that goes below 0 and above 1

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:474
(Diff revision 1)
> +
> +function testAnimationSamples(animation, idlName, options, testSamples) {
>    var target = animation.effect.target;
>    testSamples.forEach(function(testSample) {
>      animation.currentTime = testSample.time;
> -    assert_equals(getComputedStyle(target)[idlName], testSample.expected,
> +    var expected = testSample.expected.toLowerCase();

Rather than blindly doing this for every value, let's just do the lowercasing in discrete().
Attachment #8777710 - Flags: review?(bbirtles)

        Attachment #8763025 -
        Attachment is obsolete: true

        Attachment #8777710 -
        Attachment is obsolete: true

        Attachment #8778676 -
        Flags: review?(bbirtles)

    
    

    
  
Comment on attachment 8760600 [details]
Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.

https://reviewboard.mozilla.org/r/58138/#review67056

::: dom/animation/AnimValuesStyleRule.cpp:62
(Diff revision 4)
>  }
>  
> +/* virtual */ const nsCSSValue*
> +AnimValuesStyleRule::GetCSSValue(nsCSSProperty aProperty)
> +{
> +  return nullptr;

I think we need to return useful values here.  We might call StyleAnimationValue::ExtractComputedValue with a style context that is affected by animations.  Unfortunately we can't do anything better than just searching through mPropertyValuePairs, but we can use mStyleBits to avoid searching if we know we don't have any values from the struct that aProperty lives in.

::: dom/base/nsMappedAttributes.cpp:198
(Diff revision 4)
>  }
>  
> +/* virtual */ const nsCSSValue*
> +nsMappedAttributes::GetCSSValue(nsCSSProperty aProperty)
> +{
> +  return nullptr;

I guess we don't need to return useful values here right now, since I think none of the properties you convert to Discrete in this patch queue will be set by any of the nsMappedAttributes that we use:

http://searchfox.org/mozilla-central/search?q=symbol:_ZNK24nsMappedAttributeElement27GetAttributeMappingFunctionEv&redirect=false

But at some point we will have to return useful values here.

What about something like this.  We add a new RAII class called something like AssertGetCSSValueCorrect.  It is constructed with an nsIStyleRule and an nsRuleData.  We create one on the stack in nsMappedAttributes::MapRuleInfoInto -- and all the other non-StyleRule nsIStyleRules -- before we map any properties.  When constructed, it can record in an nsCSSPropertySet which properties in the nsRuleData have a non-eCSSUnit_Null value.  In its destructor, it can check that any property whose nsCSSValue is now set in the nsRuleData also gets a non-null return value from GetCSSValue.

Does that sound OK?  Is there an easier way we can assert that we're not missing anything in GetCSSValue?

::: layout/style/StyleAnimationValue.cpp:4230
(Diff revision 4)
>        aComputedValue.SetAndAdoptCSSValueListValue(result.forget(),
>                                                    eUnit_Shadow);
>        return true;
>      }
> +    case eStyleAnimType_Discrete: {
> +      nsCSSValue cssValue(eCSSUnit_Initial);

Should this be eCSSUnit_Unset, so that if we are animating an inherited property we will use the inherited value rather than the initial value?

::: layout/style/StyleAnimationValue.cpp:4231
(Diff revision 4)
> +      for (nsRuleNode* node = aStyleContext->RuleNode();
> +           node; node = node->GetParent()) {
> +        nsIStyleRule* rule = node->GetRule();
> +        if (!rule) {
> +          continue;
> +        }
> +        const nsCSSValue* value = rule->GetCSSValue(aProperty);
> +        if (!value) {
> +          continue;
> +        }
> +        cssValue = *value;
> +        break;
> +      }

Can we factor this out into a method on nsRuleNode?  I feel like it belongs there, since it walks up the tree in a similar way to nsRuleNode::WalkRuleTree.

::: layout/style/nsIStyleRule.h:55
(Diff revision 4)
> +#include "nsCSSProperty.h"
> +class nsCSSValue;

Move this #include and forward declaration up above the comment next to the other #include and forward declaration.

::: layout/style/nsIStyleRule.h:82
(Diff revision 4)
> +  /**
> +   * Returns nsCSSValue of a given property.
> +   */

Some of the nsIStyleRule implementations don't store their own nsCSSValue objects, so you won't have one that you can return a pointer to.  Instead, should we make this method take a reference to an nsCSSValue object that will be set to the given property's value?

Also, how about we make this method only work for discretely animated properties.  That way it feels more correct to be returning nullptr from, for example, BodyRule::MapRuleInfoInto (since the margin properties it does map are not discretely aniamted).  If we do that, please describe that restriction in the comment, and call the method something like GetDiscretelyAnimatedCSSValue.

::: layout/style/nsStyleSet.cpp:133
(Diff revision 4)
>  }
>  
> +/* virtual */ const nsCSSValue*
> +nsInitialStyleRule::GetCSSValue(nsCSSProperty aProperty)
> +{
> +  return nullptr;

I think we need to return valid values from nsInitialStyleRule, since in ComputeValuesFromStyleRule we use nsInitialStyleRule and then call ExtractComputedValue:

http://searchfox.org/mozilla-central/rev/389a3e0817b873a64376ec2b65f6807afbba9d8f/layout/style/StyleAnimationValue.cpp#2820

This one should be easy; just always return an eCSSUnit_Initial value.

        Attachment #8760600 -
        Flags: review?(cam) → review-

    
    

    
  
My overall comment for that patch got lost, unfortunately.  It was something like:  "This is looking better, thanks!  I think this GetCSSValue() approach will work.  Some comments below on assertions we should add and some missing GetCSSValue() implementations we need."

    
    

    
  
Brian, is it OK if I redirect the part 4, 5 and 7 patches to you?
Flags: needinfo?(bbirtles)

    
    

    
  
Comment on attachment 8778676 [details] [diff] [review]
flexbox-1277433-6.patch

Review of attachment 8778676 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following changes

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html
@@ +485,5 @@
> +
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var keyframes = {};
> +      keyframes[idlName] = ["visible", "collapse"];

See comment 83, what we want to test here is 'hidden' to 'collapse' since the special behavior only applied when one of the values is 'visible'. For 'hidden' -> 'collapse' the value should change at the 50% point.

@@ +507,5 @@
> +                                       easing: "cubic-bezier(0.68,0,1,0.01)" });
> +      testAnimationSamples(animation, idlName,
> +                           [{ time: 0,    expected: "visible" },
> +                            { time: 999,  expected: "visible" },
> +                            { time: 1000, expected: "hidden" }]);

The whole point of this test is to check what happens when the progress is < 0 and > 1. This curve doesn't put the progress outside out of that range. We need to do that, particularly with a curve that overshoots at the 'hidden' end of the range--we should report 'hidden' when the progress overshoots there.

::: testing/web-platform/tests/web-animations/testcommon.js
@@ +48,5 @@
> +// create element of given tagName, appends it to the document body and
> +// removes the created element during test cleanup
> +// if tagName is null or undefined, returns div element
> +function createTestElement(test, tagName) {
> +  return createElement(test, tagName || 'div', document);

It looks like we don't really need this. Let's drop it and just keep createElement and do the tagName = tagName || 'div' there.

        Attachment #8778676 -
        Flags: review?(bbirtles) → review+

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #87)
> Brian, is it OK if I redirect the part 4, 5 and 7 patches to you?

Yes, that's fine.
Flags: needinfo?(bbirtles)

        Attachment #8777708 -
        Flags: review?(cam) → review?(bbirtles)

        Attachment #8777709 -
        Flags: review?(cam) → review?(bbirtles)

        Attachment #8777711 -
        Flags: review?(cam) → review?(bbirtles)

    
    

    
  


  
Hi Cameron,

Based on your comment, I inserted MOZ_ASSERT into all of nsIStyleRule::GetDiscretelyAnimatedCSSValue that does not implement, then I pushed to try server to investigate when nsIStyleRule::GetDiscretelyAnimatedCSSValue (e.g. nsMappedAttributes) are called.
As result, nsMappedAttributes, HTMLBodyElement::BodyRule and AnimValuesStyleRule are called at the test cases that I attached. And other GetDiscretelyAnimatedCSSValue weren't called. Also, all of those methods were called by css-transition.

I discussed with Brian, he suggested that we may be able to avoid to extract since css-transition does not support discrete animation currently. In particular, appends a flag (e.g. isCSSTransitionAnimation) to ExtractComputedValues. Then returns false if the flag is true even if the type of CSSProperty is discetable.

I will try to implement above way. But If you have another thinking, please let me know.

Thanks!

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Daisuke Akatsuka (:daisuke) from comment #97)
> Comment on attachment 8760600 [details]
> Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment
> properties.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/58138/diff/4-5/

I changed the way that avoids extracting CSS Transition discrete animation. That codes are in nsTransitionManager.cpp

Thanks.

    
    

    
  
Comment on attachment 8760600 [details]
Bug 1277433 - Part 1: Use discrete animation for appropriate CSS Alignment properties.

https://reviewboard.mozilla.org/r/58138/#review68572

Thanks for trying that approach, Daisuke.  This looks good, just a couple of comments below.

::: layout/style/StyleAnimationValue.cpp:4221
(Diff revisions 4 - 5)
> -      nsCSSValue cssValue(eCSSUnit_Initial);
> -      for (nsRuleNode* node = aStyleContext->RuleNode();
> -           node; node = node->GetParent()) {
> +      nsCSSValue cssValue(eCSSUnit_Unset);
> +      aStyleContext->RuleNode()->GetDiscretelyAnimatedCSSValue(aProperty,
> +                                                               cssValue);
> -        nsIStyleRule* rule = node->GetRule();
> -        if (!rule) {
> -          continue;
> -        }
> -        const nsCSSValue* value = rule->GetCSSValue(aProperty);
> -        if (!value) {
> -          continue;
> -        }
> -        cssValue = *value;
> -        break;
> -      }
>        auto copiedCSSValue = MakeUnique<nsCSSValue>(cssValue);

Instead of making a copy of the nsCSSValue we allocated on the stack, can we instead just make the new nsCSSValue in the UniquePtr and then pass it through to GetDiscretelyAnimatedCSSValue?

::: layout/style/nsIStyleRule.h:17
(Diff revisions 4 - 5)
>  #define nsIStyleRule_h___
>  
>  #include <stdio.h>
>  
>  #include "nsISupports.h"
> +#include "nsCSSProperty.h"

Nit: Move this one line up to keep the #includes sorted.

::: layout/style/nsTransitionManager.cpp:287
(Diff revision 5)
>  
>  NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(nsTransitionManager, AddRef)
>  NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(nsTransitionManager, Release)
>  
> +static inline bool
> +ExtractComputedValue(nsCSSProperty aProperty, nsStyleContext* aStyleContext,

Can you give this a different name, so we don't confuse its behaviour with StyleAnimationValue::ExtractComputedValue?  Maybe "ExtractNonDiscreteComputedValue"?

        Attachment #8760600 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8777708 [details]
Bug 1277433 - Part 4: Add CSS Animation tests.

https://reviewboard.mozilla.org/r/69210/#review68612

r=me with the following changes although I suspect this might need changes to the underlying patches to get these tests to pass with those changes made.

Can we call this test {file_,test}_underlying-discrete-value.html ?

Also, I guess this should go under &#39;mozilla&#39;? I think the tests under &#39;css-animations&#39; are ones that we eventually want to put in web-platform-tests once CSS Animations 2 is specced (and CSS moves their tests to web-platform-tests), but this feels like a Gecko-specific test?

::: dom/animation/test/css-animations/file_discrete-animation.html:7
(Diff revision 2)
> +<meta charset=utf-8>
> +<script src="../testcommon.js"></script>
> +<body>
> +<script>
> +"use strict";
> +

Can we add a comment like:

// Tests that we correctly extract the underlying value when the animation
// type is 'discrete'.

::: dom/animation/test/css-animations/file_discrete-animation.html:18
(Diff revision 2)
> +    },
> +    expectedKeyframes: [
> +      { computedOffset: 0, alignContent: "flex-start" },
> +      { computedOffset: 1, alignContent: "flex-end" }
> +    ],
> +    explanation: "Test for full specifiec keyframes"

s/full specifiec/fully-specified/

::: dom/animation/test/css-animations/file_discrete-animation.html:26
(Diff revision 2)
> +    stylesheet: {
> +      "@keyframes keyframes": "from { align-content: flex-start; }"
> +    },
> +    expectedKeyframes: [
> +      { computedOffset: 0, alignContent: "flex-start" },
> +      { computedOffset: 1, alignContent: "unset" }

Shouldn't this be 'stretch'?

Currently if you have:

@keyframes {
  from { left: '100px' }
}

You'll get keyframes as:

[
  { computedOffset: 0, left: '100px' },
  { computedOffset: 1, left: 'auto' },
]

i.e. the initial value 'auto' is returned instead of 'unset'.

Likewise,

#target {
  left: 'inherit'
}
@keyframes {
  from { left: '100px' }
}

will also give you:

[
  { computedOffset: 0, left: '100px' },
  { computedOffset: 1, left: 'auto' },
]

i.e. *not* 'inherit'.

'unset' might be better, but whatever we do we need to make it consistent (so that we can specify it). How hard is it to make this return 'stretch' here?

::: dom/animation/test/css-animations/file_discrete-animation.html:35
(Diff revision 2)
> +  {
> +    stylesheet: {
> +      "@keyframes keyframes": "to { align-content: flex-end; }"
> +    },
> +    expectedKeyframes: [
> +      { computedOffset: 0, alignContent: "unset" },

Likewise, shouldn't this be 'stretch' ?

::: dom/animation/test/css-animations/file_discrete-animation.html:43
(Diff revision 2)
> +    explanation: "Test for 100% keyframe only"
> +  },
> +  {
> +    stylesheet: {
> +      "@keyframes keyframes": "50% { align-content: center; }",
> +      "#target": "align-content: stretch;"

'stretch' is the initial value for align-content so once we adjust the above tests to use 'stretch' we will no longer be able to differentiate between getting back the initial value and the specified style. We should use something else like 'space-between' here and elsewhere in this file where we specify style.

::: dom/animation/test/css-animations/file_discrete-animation.html:50
(Diff revision 2)
> +    expectedKeyframes: [
> +      { computedOffset: 0, alignContent: "stretch" },
> +      { computedOffset: 0.5, alignContent: "center" },
> +      { computedOffset: 1, alignContent: "stretch" }
> +    ],
> +    explanation: "Test for no edge keyframes and target element has the style"

"Test for no 0%/100% keyframes and specified style on target element"

::: dom/animation/test/css-animations/file_discrete-animation.html:64
(Diff revision 2)
> +    explanation: "Test for no edge keyframes " +
> +                 "and target element has style attribute"

"Test for no 0%/100% keyframes and specified style on target element using style attribute"

::: dom/animation/test/css-animations/file_discrete-animation.html:73
(Diff revision 2)
> +      { computedOffset: 0, alignContent: "inherit" },
> +      { computedOffset: 0.5, alignContent: "center" },
> +      { computedOffset: 1, alignContent: "inherit" }

These should be 'stretch', not 'inherit'.

::: dom/animation/test/css-animations/file_discrete-animation.html:77
(Diff revision 2)
> +    explanation: "Test for no edge keyframes " +
> +                 "and target element has 'inherit' style"

"Test for no 0%/100% keyframes and 'inherit' specified on target element"

::: dom/animation/test/css-animations/file_discrete-animation.html:93
(Diff revision 2)
> +    explanation: "Test for no edge keyframes " +
> +                 "and target element has the style by class"

"Test for no 0%/100% keyframes and specified style on target element using class selector"

::: dom/animation/test/css-animations/file_discrete-animation.html:106
(Diff revision 2)
> +    explanation: "Test for no edge keyframes " +
> +                 "and target element has the style by div"

"Test for no 0%/100% keyframes and specified style on target element using type selector"

::: dom/animation/test/css-animations/file_discrete-animation.html:124
(Diff revision 2)
> +    explanation: "Test for no edge keyframes " +
> +                 "and target element has styles"

"Test for no 0%/100% keyframes and specified style on target element using ID selector that overrides class selector"

::: dom/animation/test/css-animations/file_discrete-animation.html:142
(Diff revision 2)
> +    explanation: "Test for no edge keyframes " +
> +                 "and target element has important style"

"Test for no 0%/100% keyframes and specified style on target element using important type selector that overrides other rules"

::: dom/animation/test/css-animations/file_discrete-animation.html:162
(Diff revision 2)
> +    div.style.animation = "keyframes 100s";
> +
> +    const keyframes = div.getAnimations()[0].effect.getKeyframes();
> +    const expectedKeyframes = testcase.expectedKeyframes;
> +    assert_equals(keyframes.length, expectedKeyframes.length,
> +                  `keyframes.length shoud be ${ expectedKeyframes.length }`);

s/shoud/should/

::: dom/animation/test/css-animations/file_discrete-animation.html:167
(Diff revision 2)
> +                    `computedOffset of keyframes[${ index }] shoud be ` +
> +                    `${ expectedKeyframe.computedOffset }`);
> +      assert_equals(keyframe.alignContent, expectedKeyframe.alignContent,
> +                    `alignContent of keyframes[${ index }] shoud be ` +

s/shoud/should/

        Attachment #8777708 -
        Flags: review?(bbirtles) → review+

    
    

    
  
Comment on attachment 8777709 [details]
Bug 1277433 - Part 5: Replace eStyleAnimType_EnumU8 to eStyleAnimType_Discrete.

https://reviewboard.mozilla.org/r/69212/#review68616

r=me with the following change:

::: layout/style/StyleAnimationValue.cpp:4222
(Diff revision 2)
> +        aComputedValue.SetIntValue(aStyleContext->StyleVisibility()->mVisible,
> +                                   eUnit_Visibility);

I think this can just be:

  aComputedValue.SetIntValue(
    static_cast<const nsStyleVisibility*>(styleStruct)->mVisible,
    eUnit_Visibility);

        Attachment #8777709 -
        Flags: review?(bbirtles) → review+

    
    

    
  
Comment on attachment 8777710 [details]
Bug 1277433 - Part 6: Add tests that were eStyleAnimType_EnumU8.

https://reviewboard.mozilla.org/r/69214/#review68620

        Attachment #8777710 -
        Flags: review?(bbirtles) → review+

    
    

    
  
Comment on attachment 8777711 [details]
Bug 1277433 - Part 7: Remove eStyleAnimType_EnumU8 related codes.

https://reviewboard.mozilla.org/r/69216/#review68622

        Attachment #8777711 -
        Flags: review?(bbirtles) → review+

    
    

    
  
I wonder what we should do about returning 'unset' / 'initial' from getKeyframes().

It seems like that for regular interpolable values we resolve 'initial' etc. to their computed value. That makes sense since this method is called "ExtractComputedValue" and 'initial' etc. are not computed values. However, for discrete values we're returning 'unset' / 'initial' etc.

This is inconsistent but I'm not sure which is right. Firstly, the spec seems to indicate we should snapshot the computed values when we create an animation:

'If a 0% or from keyframe is not specified, then the user agent constructs a 0% keyframe using the computed values of the properties being animated. If a 100% or to keyframe is not specified, then the user agent constructs a 100% keyframe using the computed values of the properties being animated.'[1]

I *thought* all the browsers agreed on this but testing now[2] I notice that Chrome seems to reflect changes to the underlying style. I'm pretty sure it didn't last time I checked. Both Gecko and Edge, however, do not. I don't have access to Safari to check it, however.

Adding a test for a discretely animated property[3], flex-direction, I notice that Chrome also reflects changes to the underlying style for these properties too.

If we think the Chrome approach is better in the long-term, then we should land this as-is and fix the other animation types to support 'initial'-type values. Alternatively, we could just wait until additive animations lands and just use that mechanism since this would basically be equivalent to what the Web Animations API says to do for missing 0.0/1.0 keyframes.

I'm not sure what the consequences of this after for transitions, however, since I think we use this same method for the starting point of transitions? Would it be ok if the starting point of the transition was updated mid-transition?

It seems like when we come to do additive animations we'll need to get the computed underlying value (so we can interpolate it) and that transitions we should probably use the computed value. So perhaps the best thing here is to change this to get the computed value?

(The other issue is that with these patches is that getKeyframes() returns 'initial' and 'unset' for discretely animated values but not for other animation types. That's not a real problem, however, since we won't be exposing CSS animations through the Web Animations API for a while yet.)

[1] https://drafts.csswg.org/css-animations/#keyframes
[2] https://jsfiddle.net/mn86oanb/
[3] https://jsfiddle.net/mn86oanb/1/

    
    

    
  
Cameron, what do you think about comment 110? I'm inclined to say we should just land this as-is since it is a real edge case (updating an auto-generated discretely-animated value) and then just make it return the computed value in a separate bug.
Flags: needinfo?(cam)

    
    

    
  
I think it's fine to get this in as is and work out the details of how generated "initial" values for discrete animations are exposed later.
Flags: needinfo?(cam)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f77b7188d3b2
Part 1: Use discrete animation for appropriate CSS Alignment properties. r=dholbert,heycam
https://hg.mozilla.org/integration/autoland/rev/6fb73a2de6e8
Part 2: Add tests for CSS Alignment. r=birtles
https://hg.mozilla.org/integration/autoland/rev/456786b221f4
Part 3: Add tests for CSS Flexbox. r=birtles
https://hg.mozilla.org/integration/autoland/rev/69bda5f5e1c0
Part 4: Add CSS Animation tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/20735d766362
Part 5: Replace eStyleAnimType_EnumU8 to eStyleAnimType_Discrete. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ecbbf3ee7a0d
Part 6: Add tests that were eStyleAnimType_EnumU8. r=birtles
https://hg.mozilla.org/integration/autoland/rev/6d587f3b5f0a
Part 7: Remove eStyleAnimType_EnumU8 related codes. r=birtles

    
    

    
  
When I attempt to compile, I get this error:

obj-i686-pc-mingw32\dist\include\nsRuleNode.h(864):
  error C2061: syntax error: identifier 'nsCSSProperty'
obj-i686-pc-mingw32\dist\include\nsIStyleRule.h(16):
  fatal error C1083: Cannot open include file: 'nsCSSProperty.h': No such file or directory

Seems due to https://hg.mozilla.org/mozilla-central/rev/f77b7188d3b2

Am I doing something wrong?

    
    

    
  
That's due to merge conflicts between this bug and bug 1293739.  If you update your tree, you should get a backout of bug 1293739 which should hopefully fix this for you.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: