Closed Bug 1286150 Opened 8 years ago Closed 8 years ago

Support paced spacing for basic shapes

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: boris, Assigned: boris)

References

(
URL
)

Details

Attachments

(6 files)

Bug 1286150 - Part 1: Simplify AddTransformTranslate and reuse AddCSSValuePixelPercentCalc.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1286150 - Part 2: Support difference restrictions on AddShapeFunction.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1286150 - Part 3: Implement shape distance for circle and ellipse.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1286150 - Part 4: Implement shape distance for polygon.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1286150 - Part 5: Implement shape distance for inset.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1286150 - Part 6: Test.
58 bytes, text/x-review-board-request
hiro
: review+
Details 
Bug 1244590 implemented paced spacing, and we also need to implement StyleAnimationValue::ComputeDistance() for basic shapes [1]. According to Bug 1272549 comment 1 and Bug 1272549 comment 2, the current spec [2] doesn't give us any formula for calculating the distance between two shape functions. so we can follow the same rule for interpolation and could use the square root of the sum of the squares of distances of each components between two interpolatable basic shapes.

[1] https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/layout/style/StyleAnimationValue.cpp#864-867
[2] https://drafts.csswg.org/css-shapes/#basic-shape-interpolation
Assignee: nobody → boris.chiou
Just notice that there are some duplicated static functions in StyleAnimationValue.cpp

e.g.
AddCSSValuePixelPercentCalc[1] and AddTransformTranslate[2] are similar. I guess this bug may need AddCSSValuePixelPercentCalc, so maybe we can also merge them here. 

[1] http://searchfox.org/mozilla-central/rev/e8dd21b1673c07307068303ab3e99360c78f4d12/layout/style/StyleAnimationValue.cpp#1158-1186
[2] http://searchfox.org/mozilla-central/rev/e8dd21b1673c07307068303ab3e99360c78f4d12/layout/style/StyleAnimationValue.cpp#1347-1371
Status: NEW → ASSIGNED
Hi Brian, Hiro,

It's time to talk about my proposal about computing the distance between two basic shapes. I think we can follow the same rules as what we do for interpolation.

1. We only calculate the distance between two _same_ basic shapes functions,
   and both shape functions use the _same_ reference box.
   For inset shape function, we compute the distance only if the fill rule is the same.

2. There are 4 types of basic shapes: circle, ellipse, polygon, and inset [1],
   and we support both <length-percent> and <calc> types for their parameters, so my idea is simple:
   a) Reuse AddShapeFunction(1.0, array2, -1.0, array1, ...) [2] to create a temp difference shape function.
      If we cannot get this function, we return 0.0.
   b) Calculate the distance by euclidean distance.
      e.g. shape1:  { clipPath: "circle(calc(10px + 10px) at 20px 10px)" }
           shape2:  { clipPath: "circle(calc(20px + 30px) at 10px 50%)" },

      * step 1: we get the temp difference shape function: "circle(calc(30px) at -10px calc(50% - 10px))"
      * step 2: the answer: sqrt( 30 * 30 + (-10) * (-10) + 0.5 * 0.5 + (-10) * (-10) )
      * Note: We use computed values, so 50% is 0.5 in this example.

[1] https://drafts.csswg.org/css-shapes/#supported-basic-shapes
[2] http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/layout/style/StyleAnimationValue.cpp#2154-2157


Hiro, could you please review my patches?
Brian, if there is no other comments for this proposal, I will follow the above rules because I think they may be the most reasonable way to get the distance between two basic shapes.

Thanks.
Attachment #8805452 - Flags: review?(hiikezoe)
Thanks for doing this. That plan sounds good to me. We need to get this into a spec too I suppose but since we're not shipping it yet that doesn't need to block landing this. If you have time, could you file an issue on the appropriate spec (perhaps with proposed text?) and @mention (@birtles) me? Thanks!
Sounds quite reasonable to me.

(In reply to Boris Chiou [:boris] from comment #8)
>       e.g. shape1:  { clipPath: "circle(calc(10px + 10px) at 20px 10px)" }
>            shape2:  { clipPath: "circle(calc(20px + 30px) at 10px 50%)" },
> 
>       * step 1: we get the temp difference shape function:
> "circle(calc(30px) at -10px calc(50% - 10px))"
>       * step 2: the answer: sqrt( 30 * 30 + (-10) * (-10) + 0.5 * 0.5 +
> (-10) * (-10) )
>       * Note: We use computed values, so 50% is 0.5 in this example.

One thing I am concerned is that 0.5 is relatively smaller than others.  If the position of the shape1 is 'at 20px 20%', the difference value is 0.3 * 0.3?
Comment on attachment 8805447 [details]
Bug 1286150 - Part 1: Simplify AddTransformTranslate and reuse AddCSSValuePixelPercentCalc.

https://reviewboard.mozilla.org/r/88928/#review88324

::: layout/style/StyleAnimationValue.cpp:614
(Diff revision 1)
> -  MOZ_ASSERT(aValue1.GetUnit() == eCSSUnit_Percent ||
> -             aValue1.GetUnit() == eCSSUnit_Pixel ||
> -            aValue1.IsCalcUnit(),
> -            "unexpected unit");
> -  MOZ_ASSERT(aValue2.GetUnit() == eCSSUnit_Percent ||
> -             aValue2.GetUnit() == eCSSUnit_Pixel ||
> -             aValue2.IsCalcUnit(),
> -             "unexpected unit");
> +  switch (aCommonUnit) {
> +    case eCSSUnit_Pixel:
> +      AddCSSValuePixel(aCoeff1, aValue1,
> +                       aCoeff2, aValue2,
> +                       aResult, aValueRestrictions);
> +      break;
> +    case eCSSUnit_Percent:
> +      AddCSSValuePercent(aCoeff1, aValue1,

I think we should leave an assert for aValue1.IsCalcUnit() || aValue2.IsCalcUnit(), otherwise we will call AddCSSValuePixelPercentCalc()   when neither aValue1 nor aValue2 has calc unit. No?
Attachment #8805447 - Flags: review?(hiikezoe) → review+
Comment on attachment 8805448 [details]
Bug 1286150 - Part 2: Support difference restrictions on AddShapeFunction.

https://reviewboard.mozilla.org/r/88930/#review88330

::: layout/style/StyleAnimationValue.cpp:2147
(Diff revision 1)
>  static already_AddRefed<nsCSSValue::Array>
>  AddShapeFunction(nsCSSPropertyID aProperty,
>                   double aCoeff1, const nsCSSValue::Array* aArray1,
> -                 double aCoeff2, const nsCSSValue::Array* aArray2)
> +                 double aCoeff2, const nsCSSValue::Array* aArray2,
> +                 bool aDisableRestrictions = false)

I am not a big fan of boolean argument.  Can we use enum instead of bool?  Or can we pass 'const uint32_t aValueRestrictions' as the first argument just like AddCSSValuePixelPercentCalc() does?
Though I did not notice while I was reviewing tests for spacing for transform, should we move tests for spacing into testing/web-platform/tests/web-animations/animation-model/animation-types?
(In reply to Brian Birtles (:birtles, high review load, away most of 27 Oct-4 Nov) from comment #9)
> Thanks for doing this. That plan sounds good to me. We need to get this into
> a spec too I suppose but since we're not shipping it yet that doesn't need
> to block landing this. If you have time, could you file an issue on the
> appropriate spec (perhaps with proposed text?) and @mention (@birtles) me?
> Thanks!

OK. I would like to file an issue to css-shape-1, (https://github.com/w3c/csswg-drafts/labels/css-shapes-1) with my proposed text.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> Sounds quite reasonable to me.
> 
> (In reply to Boris Chiou [:boris] from comment #8)
> >       e.g. shape1:  { clipPath: "circle(calc(10px + 10px) at 20px 10px)" }
> >            shape2:  { clipPath: "circle(calc(20px + 30px) at 10px 50%)" },
> > 
> >       * step 1: we get the temp difference shape function:
> > "circle(calc(30px) at -10px calc(50% - 10px))"
> >       * step 2: the answer: sqrt( 30 * 30 + (-10) * (-10) + 0.5 * 0.5 +
> > (-10) * (-10) )
> >       * Note: We use computed values, so 50% is 0.5 in this example.
> 
> One thing I am concerned is that 0.5 is relatively smaller than others.  If
> the position of the shape1 is 'at 20px 20%', the difference value is 0.3 *
> 0.3?

Yes. Because we use "computed values", instead of "used values". This might be a problem [1], but we won't fix it now [2]. Therefore, I calculate the distance by the same rules we used for eUnit_Calc.

[1] Bug 1276193 - Use used values to compute distance for keyframe spacing
[2] https://github.com/w3c/web-animations/commit/87ad252447423f9ecbe8e787e1c7552764642734
[3] http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/layout/style/StyleAnimationValue.cpp#1135-1142
Comment on attachment 8805448 [details]
Bug 1286150 - Part 2: Support difference restrictions on AddShapeFunction.

https://reviewboard.mozilla.org/r/88930/#review88330

> I am not a big fan of boolean argument.  Can we use enum instead of bool?  Or can we pass 'const uint32_t aValueRestrictions' as the first argument just like AddCSSValuePixelPercentCalc() does?

We use two different way to get the Restricitons in this functions.
1. Use CSS_PROPERTY_VALUE_NONNEGATIVE directly
   http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/layout/style/StyleAnimationValue.cpp#2189,2201
2. Use nsCSSProps::ValueRestrictions(eCSSProperty_border_top_left_radius) to get the Restrictions of radii
   http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/layout/style/StyleAnimationValue.cpp#2263-2264
   
So I use a bool to check it at first. Your suggestion is good, and I'd like to use enum class to replace the boolean argument.
Comment on attachment 8805449 [details]
Bug 1286150 - Part 3: Implement shape distance for circle and ellipse.

https://reviewboard.mozilla.org/r/88932/#review88340

::: layout/style/StyleAnimationValue.cpp:749
(Diff revision 1)
> +  auto pixelCalcDistance = [](const PixelCalcValue& aValue) {
> +    return aValue.mLength * aValue.mLength + aValue.mPercent * aValue.mPercent;
> +  };

I think we should check mHasPercent member (in MOZ_ASSERT?) just in case.

::: layout/style/StyleAnimationValue.cpp:755
(Diff revision 1)
> +  const size_t len = func->Count();
> +  nsCSSKeyword shapeFuncName = func->Item(0).GetKeywordValue();
> +  switch (shapeFuncName) {
> +    case eCSSKeyword_ellipse:
> +    case eCSSKeyword_circle:

Please move 'len' inside the case block. It will not be used for other cases (as far as I see part 4 and part 5).

::: layout/style/StyleAnimationValue.cpp:1481
(Diff revision 1)
>      case eUnit_Shape:
> -      // Bug 1286150: The CSS Shapes spec doesn't define paced animations for
> -      // shape functions, but we still need to implement one.
> -      return false;
> +      aDistance = ComputeShapeDistance(aProperty,
> +                                       aStartValue.GetCSSValueArrayValue(),
> +                                       aEndValue.GetCSSValueArrayValue());
> +      return true;

I think this part should be done in the last patch because at this point neither polygon nor inset works.
Attachment #8805449 - Flags: review?(hiikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Though I did not notice while I was reviewing tests for spacing for
> transform, should we move tests for spacing into
> testing/web-platform/tests/web-animations/animation-model/animation-types?

Actually, I also have this question. These formulas of computing distances on transform, shape, and filter are not in the spec, so I put those test cases in dom/animation/test/mozilla/. If we have an official spec for these distance, it would be better to put them in wpt. Hi, Brian, should I move them into wpt?
Flags: needinfo?(bbirtles)
Comment on attachment 8805450 [details]
Bug 1286150 - Part 4: Implement shape distance for polygon.

https://reviewboard.mozilla.org/r/88934/#review88346
Attachment #8805450 - Flags: review?(hiikezoe) → review+
Comment on attachment 8805450 [details]
Bug 1286150 - Part 4: Implement shape distance for polygon.

https://reviewboard.mozilla.org/r/88934/#review88348
Comment on attachment 8805451 [details]
Bug 1286150 - Part 5: Implement shape distance for inset.

https://reviewboard.mozilla.org/r/88936/#review88350
Attachment #8805451 - Flags: review?(hiikezoe) → review+
(In reply to Boris Chiou [:boris] from comment #15)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> > Sounds quite reasonable to me.
> > 
> > (In reply to Boris Chiou [:boris] from comment #8)
> > >       e.g. shape1:  { clipPath: "circle(calc(10px + 10px) at 20px 10px)" }
> > >            shape2:  { clipPath: "circle(calc(20px + 30px) at 10px 50%)" },
> > > 
> > >       * step 1: we get the temp difference shape function:
> > > "circle(calc(30px) at -10px calc(50% - 10px))"
> > >       * step 2: the answer: sqrt( 30 * 30 + (-10) * (-10) + 0.5 * 0.5 +
> > > (-10) * (-10) )
> > >       * Note: We use computed values, so 50% is 0.5 in this example.
> > 
> > One thing I am concerned is that 0.5 is relatively smaller than others.  If
> > the position of the shape1 is 'at 20px 20%', the difference value is 0.3 *
> > 0.3?
> 
> Yes. Because we use "computed values", instead of "used values". This might
> be a problem [1], but we won't fix it now [2]. Therefore, I calculate the
> distance by the same rules we used for eUnit_Calc.
> 
> [1] Bug 1276193 - Use used values to compute distance for keyframe spacing
> [2]
> https://github.com/w3c/web-animations/commit/
> 87ad252447423f9ecbe8e787e1c7552764642734
> [3]
> http://searchfox.org/mozilla-central/rev/
> 4012e61cc38758ffa1f7ce26039173f67b048853/layout/style/StyleAnimationValue.
> cpp#1135-1142

Thanks for the pointer!  And I am sorry that I did not check them at all.
Comment on attachment 8805452 [details]
Bug 1286150 - Part 6: Test.

https://reviewboard.mozilla.org/r/88938/#review88354

I am not sure we should move this file into web-platform test (it depends on Brian's answer), but test cases here seem good to me.

::: dom/animation/test/mozilla/file_spacing_shapes.html:8
(Diff revision 1)
> +// Help function for testing the computed offsets by the distance array.
> +function assert_animation_offsets(anim, dist) {
> +  const frames = anim.effect.getKeyframes();
> +  const cumDist = dist.reduce( (prev, curr) => {
> +    prev.push(prev.length == 0 ? curr : curr + prev[prev.length - 1]);
> +    return prev;
> +  }, []);
> +
> +  const total = cumDist[cumDist.length - 1];
> +  for (var i = 0; i < frames.length; ++i) {
> +    assert_equals(frames[i].computedOffset, cumDist[i] / total,
> +                  'computedOffset of frame ' + i);
> +  }
> +}
> +

I think we will end up moving this function into testcommon.js. ;-)


Do we need a test case that we can't compute distance?
Attachment #8805452 - Flags: review?(hiikezoe) → review+
(In reply to Boris Chiou [:boris] from comment #14)
> OK. I would like to file an issue to css-shape-1,
> (https://github.com/w3c/csswg-drafts/labels/css-shapes-1) with my proposed
> text.

https://github.com/w3c/csswg-drafts/issues/662
Comment on attachment 8805447 [details]
Bug 1286150 - Part 1: Simplify AddTransformTranslate and reuse AddCSSValuePixelPercentCalc.

https://reviewboard.mozilla.org/r/88928/#review88324

> I think we should leave an assert for aValue1.IsCalcUnit() || aValue2.IsCalcUnit(), otherwise we will call AddCSSValuePixelPercentCalc()   when neither aValue1 nor aValue2 has calc unit. No?

I can add these assertions back into AddTransformTranslate() to make sure we only accept these three kinds of css units, so when the common unit is eCSSUnit_Calc, aValue1 and aValue2 could be converted into PixelCalcValue correctly in AddCSSValuePixelPercentCalc().
(In reply to Boris Chiou [:boris] from comment #18)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> > Though I did not notice while I was reviewing tests for spacing for
> > transform, should we move tests for spacing into
> > testing/web-platform/tests/web-animations/animation-model/animation-types?
> 
> Actually, I also have this question. These formulas of computing distances
> on transform, shape, and filter are not in the spec, so I put those test
> cases in dom/animation/test/mozilla/. If we have an official spec for these
> distance, it would be better to put them in wpt. Hi, Brian, should I move
> them into wpt?

I think that's probably fine. We have an issue filed on the spec in question with proposed wording. I don't think Chrome does paced animation and hopefully by the time they do the spec text has been added in.
Flags: needinfo?(bbirtles)
Comment on attachment 8805449 [details]
Bug 1286150 - Part 3: Implement shape distance for circle and ellipse.

https://reviewboard.mozilla.org/r/88932/#review88340

> I think we should check mHasPercent member (in MOZ_ASSERT?) just in case.

OK. The parameter, aValue, is always returned from ExtractCalcValue(), which should make sure the mPercent and mHasPercent have correct values. I will add one more assertion here just in case:
e.g.
MOZ_ASSERT(aValue.mHasPercent || aValue.mPercent == 0.0f,
           "can't have a nonzero percentage part without having percentages");

> I think this part should be done in the last patch because at this point neither polygon nor inset works.

OK. I know your concerns, but if we implemenet ComputeShapeDistance() and use it until the last patch, I will get a compilation error, "unused function 'ComputeShapeDistance' [-Werror,-Wunused-function]", in this patch. My alternately way is adding temporary assertions for polygon and inset. Thanks.
(In reply to Brian Birtles (:birtles, high review load, away most of 27 Oct-4 Nov) from comment #27)
> (In reply to Boris Chiou [:boris] from comment #18)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> > > Though I did not notice while I was reviewing tests for spacing for
> > > transform, should we move tests for spacing into
> > > testing/web-platform/tests/web-animations/animation-model/animation-types?
> > 
> > Actually, I also have this question. These formulas of computing distances
> > on transform, shape, and filter are not in the spec, so I put those test
> > cases in dom/animation/test/mozilla/. If we have an official spec for these
> > distance, it would be better to put them in wpt. Hi, Brian, should I move
> > them into wpt?
> 
> I think that's probably fine. We have an issue filed on the spec in question
> with proposed wording. I don't think Chrome does paced animation and
> hopefully by the time they do the spec text has been added in.

Thanks, Brian. I will move this test case into testing/web-platform/tests/web-animations/animation-model/animation-types/.

And create a bug to move test_spacing_transform.html into the same directory.
Comment on attachment 8805452 [details]
Bug 1286150 - Part 6: Test.

https://reviewboard.mozilla.org/r/88938/#review88354

> I think we will end up moving this function into testcommon.js. ;-)
> 
> 
> Do we need a test case that we can't compute distance?

OK. I'd like to add two more test cases:
1. We cannot compute distance if the shape functions are different:

test(function(t) {
  var anim =
    addDiv(t).animate([ { clipPath: 'circle(20px)' },
                        { clipPath: 'ellipse(10px 20px)' },
                        { clipPath: 'polygon(50px 0px, 100px 50px, ' +
                                    '        50px 100px, 0px 50px)' },
                        { clipPath: 'inset(20px round 10px)' } ],
                      { spacing: 'paced(clip-path)' });
  const frames = anim.effect.getKeyframes();
  const slots = frames.length - 1;
  for (var i = 0; i < frames.length; ++i) {
    assert_equals(frames[i].computedOffset, i / slots,
                  'computedOffset of frame ' + i);
  }
}, 'Test falling back to distribute spacing when using different basic shapes');


2. We cannot compute distance if the reference boxes are different. (default is margin-box)

test(function(t) {
  var anim =
    addDiv(t).animate([ { clipPath: 'circle(10px)' },
                        { clipPath: 'circle(20px) content-box' },
                        { clipPath: 'circle(70px)'},
                        { clipPath: 'circle(10px) padding-box' } ],
                      { spacing: 'paced(clip-path)' });
  const frames = anim.effect.getKeyframes();
  const slots = frames.length - 1;
  for (var i = 0; i < frames.length; ++i) {
    assert_equals(frames[i].computedOffset, i / slots,
                  'computedOffset of frame ' + i);
  }
}, 'Test falling back to distribute spacing when using different ' +
   'reference boxes');
Comment on attachment 8805452 [details]
Bug 1286150 - Part 6: Test.

Hi, Hiro,

Sorry, this patch needs your review again because I added two more tests and move all into the wpt directory.

Thanks.
Attachment #8805452 - Flags: review+ → review?(hiikezoe)
Comment on attachment 8805452 [details]
Bug 1286150 - Part 6: Test.

https://reviewboard.mozilla.org/r/88938/#review89156

::: testing/web-platform/meta/MANIFEST.json:38670
(Diff revision 2)
> +        "web-animations/animation-model/animation-types/spacing-keyframes-shapes.html": [
> +          {
> +            "path": "web-animations/animation-model/animation-types/spacing-keyframes-shapes.html",
> +            "url": "/web-animations/animation-model/animation-types/spacing-keyframes-shapes.html"
> +          }
> +        ],

This is added by ./mach web-platform-tests --manifest-update xxx.
Comment on attachment 8805452 [details]
Bug 1286150 - Part 6: Test.

https://reviewboard.mozilla.org/r/88938/#review89158
Attachment #8805452 - Flags: review?(hiikezoe) → review+
Comment on attachment 8805448 [details]
Bug 1286150 - Part 2: Support difference restrictions on AddShapeFunction.

https://reviewboard.mozilla.org/r/88930/#review89160
Attachment #8805448 - Flags: review?(hiikezoe) → review+
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62fe5d16e9f3
Part 1: Simplify AddTransformTranslate and reuse AddCSSValuePixelPercentCalc. r=hiro
https://hg.mozilla.org/integration/autoland/rev/0a19bbc3c715
Part 2: Support difference restrictions on AddShapeFunction. r=hiro
https://hg.mozilla.org/integration/autoland/rev/acbcd6418399
Part 3: Implement shape distance for circle and ellipse. r=hiro
https://hg.mozilla.org/integration/autoland/rev/3f70a3328993
Part 4: Implement shape distance for polygon. r=hiro
https://hg.mozilla.org/integration/autoland/rev/1d839c998bab
Part 5: Implement shape distance for inset. r=hiro
https://hg.mozilla.org/integration/autoland/rev/2ba49ae5f6db
Part 6: Test. r=hiro
Blocks: 1317914
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: