Closed Bug 1309752 Opened 8 years ago Closed 6 years ago

Logical properties should be supported by Web Animation

Categories

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

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: daisuke, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 11 obsolete files)

Bug 1309752: Add a method to physicalize longhands.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1309752: Introduce PropertyDeclaration::to_physical.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1309752: Animate logical properties.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1309752: Update animations if the logical to physical property changes.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1309752: Cleanup might_need_transitions_update.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Tests for Web Animations and CSS Animations
25.68 KB, patch
emilio
: review+
Details | Diff | Splinter Review
Transitions tests.
3.13 KB, patch
birtles
: review+
Details | Diff | Splinter Review 
Logical properties such as border-block-start-color, border-block-start-style, border-block-start-width are animatable in the spec.
https://drafts.csswg.org/css-logical-props/#propdef-border-block-start-color
Assignee: nobody → dakatsuka
Will change the following properties to discrete animation according to the spec.

block-size, inline-size,
https://drafts.csswg.org/css-logical-props/#logical-dimension-properties

border-block-start-color, border-block-end-color, border-inline-start-color, border-inline-end-color
https://drafts.csswg.org/css-logical-props/#propdef-border-block-end-color

border-block-start-style, border-block-end-style, border-inline-start-style, border-inline-end-style
https://drafts.csswg.org/css-logical-props/#propdef-border-block-end-style

border-block-start-width, border-block-end-width, border-inline-start-width, border-inline-end-width
https://drafts.csswg.org/css-logical-props/#propdef-border-block-end-width

margin-block-start, margin-block-end, margin-inline-start, margin-inline-end
https://drafts.csswg.org/css-logical-props/#propdef-margin-block-end

max-block-size, max-inline-size
https://drafts.csswg.org/css-logical-props/#propdef-max-block-size

min-block-size, min-inline-size
https://drafts.csswg.org/css-logical-props/#propdef-min-block-size

offset-block-start, offset-block-end, offset-inline-start, offset-inline-end
https://drafts.csswg.org/css-logical-props/#propdef-offset-block-end

padding-block-start, padding-block-end, padding-inline-start, padding-inline-end
https://drafts.csswg.org/css-logical-props/#propdef-padding-block-end
I haven't looked closely at the patches yet, but why do we use discrete animation for the color- and length-typed logical properties?
Flags: needinfo?(dakatsuka)
(Or: why does the spec say to animate these discretely?)
Yeah, Daisuke and I looked into this and it looks like the bikeshed template is automatically generating "Animation type: discrete". On the blink side, it seems like noone ever set interpolable: true on CSSProperties.json5 so it ended up being discrete. I don't think this is intentional and I also agree block-size should animate as a length.

I think it might make most sense to convert, say, block-size to 'height' or 'width' at roughly the same point we expand shorthands. That conversion will depend on the writing mode which could be set in the same keyframe but I think we already have a similar problem with font-size and with custom properties too so we'll never to solve that order dependency at some time anyway.
We will need to update Web Animations to say that if, for example, 'block-size' and 'height' are specificied on the same keyframe (where block-size corresponds to height) then the physical declaration wins. For CSS @keyframes I expect the regular CSS declaration block processing should apply (i.e. last one wins).
Thanks Cameron and Brian.

I will fix to animate with same type of the physical property which the logical property represents.
Also, I forgot to correspond to missing keyframes.
Will fix this as well.
So, please let me cancel the review once.
Flags: needinfo?(dakatsuka)
Attachment #8853287 - Flags: review?(cam)
Attachment #8853288 - Flags: review?(bbirtles)
If there is like following animation,

@keyframes anim {
  from {
    block-size: 100px;
    writing-mode: vertical-lr;
  }
  to {
    block-size: 500px;
    writing-mode: horizontal-tb;
  }
}

div {
  width: 10px;
  height: 50px;
  animation: anim 1s;
}

is the following animation result expected?

| offset |   0   |   1   |
--------------------------
| width  | 100px | 10px  |
| height | 50px  | 500px |
Daisuke and I have been talking about how to handle changes to the writing mode. Roughly, the problem is this:

  elem.animate([ { blockSize: '200px' },
                 { writingMode: 'inherit',
                   offset: 0,
                   easing: 'ease-in' },
                 { blockSize: '300px',
                   writingMode: 'vertical-rl',
                   height: '400px' } ],
                2000);

The way this works is that initially the writing-mode is 'inherit' which, let's assume, is 'horizontal-tb'
here. As a result, block-size basically aliases 'height'. I think, therefore, the interpolations we want to do
are, in effect:

  height: 200px -> 400px
  writing-mode: inherit -> vertical-rl

I'm basing this interpretation partly on the spec which says, "the logical and physical properties share
computed values" and partly on the fact that our implementation of getComputedStyle() returns an empty string
for logical properties. So I'm basically assuming that animation happens in computed value space (roughly
speaking) and that at that point we have resolved logical properties to physical ones.

Also, note that normally the precendence between logical and physical properties is based on the CSS cascade.
Within a single declaration block, that means last one wins. For the API where we don't have ordering between
Javascript object properties we will need to make a rule that says, e.g. 'physical beats logical'.

Now, the tricky part with the above example is that about something like the 60% mark the writing-mode will
change to 'vertical-cl' (the 50% flip behavior except that we have a timing function here that makes it closer
to 60%). As a result, block-size will now correspond to 'width'. So the interpolations we are left with are:

  height: <underlying> -> 400px
  width: 200px -> 300px
  writing-mode: inherit -> vertical-rl

Resolving these logical properties and combining them with physical properties is complex. It seems like it
would be easiest done as a separate step just like we do for shorthands, i.e. as part of updating the
properties (that is, the mProperties member).

The tricky part, however, is that this can happen as part of a tick. Ideally we'd like to do it in
KeyframeEffectReadOnly::ComposeStyle. However, that's problematic for Stylo since we don't want to mutate
KeyframeEffectReadOnly during regular restyling.

It seems like we have a few options:

1) Try to detect this case in advance as part of the pre-traversal.
   This seems like the best approach but, after further, thought I'm not sure it's right. If writing-mode
   inherits, we won't be able to calculate this correctly in the pre-traversal and will need to do
   one of the following options anyway.

2) Detect this case in ComposeStyle and trigger an UpdateEffects task (followed by another traversal).
   This seems pretty good except that if we want to extend this approach to handling animation-related changes
   to font-size then we will basically end up doing this (two traversals) on every single frame of the
   animation.

   Perhaps for font-size, however, we should take another approach. For example, we could simply em-based
   values as-is in our animation values and then either:

   a) Interpolate font-size animations first (i.e. across all effects targeting an element and across both
      transitions and cascade levels of the cascade), then, when interpolating other types that take
      font-sizes, pass in the current font-size and resolve em units as part of interpolating.

   b) Interpolate properties in any order, but for properties with em-based units produce calc() expressions.

3) Detect if we need updated properties in ComposeStyle, and, if we do, generate them there, use them to
   interpolate, then pass the updated properties as a parameter to a SequentialTask where we use them to
   update mProperties. The main difference is that we don't need to do a subsequent traversal in this case.

I think I lean towards (2a) at this stage but if (3) is possible, it seems attractive too. I'm not sure which of these approaches will better work with custom properties, however.

Hiro, what do you think?
Flags: needinfo?(hikezoe)
I agree. (3) is very attractive and it seems to be feasible to me. One thing I am concerned is that the new SequentialTask to update mProperties will be processed between animation-only restyles and normal restyles.  I think it should work but we never do process SequentialTask between these restyle processes for now.  Currently we only process SequentialTask(s) after normal restyles.
Flags: needinfo?(hikezoe)
I talked with Brian about a way to implement this.
At first, implement only writing-mode on Gecko side according to (2a). 
# This includes a meaning to confirm the algorithm.
Then, will implement Stylo side.
Also, the following animation

@keyframes anim {
  from {
    block-size: 100px;
    writing-mode: vertical-lr;
  }
  to {
    block-size: 10px;
    writing-mode: horizontal-tb;
  }
}

div {
  width: 200px;
  height: 20px;
  animation: anim 1s;
}

expects as following result about:

| offset |   0   |  0.25 |0.49999|  0.5  |0.51111|  0.75 |   1   |
------------------------------------------------------------------
| width  | 100px | 125px |149.9px| 200px | 200px | 200px | 200px |
| height |  20px |  20px |  20px |  15px | 14.9px| 12.5px|  10px |
Comment on attachment 8853287 [details]
Bug 1309752 - Part 1: Take out a method which returns physical property from logical property, to use from other places.

https://reviewboard.mozilla.org/r/125376/#review137100

::: layout/generic/WritingModes.h:508
(Diff revision 2)
>    void InitFromStyleVisibility(const nsStyleVisibility* aStyleVisibility)
>    {
> -    switch (aStyleVisibility->mWritingMode) {
> +    mWritingMode = WritingModeFromStyles(aStyleVisibility->mWritingMode,
> +                                         aStyleVisibility->mTextOrientation,
> +                                         aStyleVisibility->mDirection);
> +  }
> +
> +  static uint8_t WritingModeFromStyles(uint8_t aWritingMode,
> +                                       uint8_t aTextOrientation,
> +                                       uint8_t aDirection)

For consistency, maybe WritingModeFromStyles() should be a non-static method InitFromStyles() that returns void?
Attachment #8853287 - Flags: review?(cam) → review+
Comment on attachment 8853288 [details]
Bug 1309752 - Part 2: Physicalize logical animation properties for Gecko.

https://reviewboard.mozilla.org/r/125378/#review137174

This patch needs a descriptive commit message. I don't understand why we need a new animation type. I thought we were converting properties to physical properties when converting from keyframes (nsCSSValues) to property arrays (StyleAnimationValues). If StyleAnimationValue objects are always physical properties, why do we need a logical animation type?

::: dom/animation/KeyframeEffectReadOnly.cpp:329
(Diff revision 2)
> +PhysicalizeLogicalProperties(nsTArray<AnimationProperty>& aProperties,
> +                             nsStyleContext* aStyleContext)

Perhaps call this just MakePropertiesPhysical ?

Or even "EnsurePhysicalProperties" for consistency with nsCSSDataBlock.cpp etc.

::: dom/animation/KeyframeEffectReadOnly.cpp:332
(Diff revision 2)
> +  uint8_t writingMode = aStyleContext->StyleVisibility()->mWritingMode;
> +  uint8_t textOrientation = aStyleContext->StyleVisibility()->mTextOrientation;
> +  uint8_t direction = aStyleContext->StyleVisibility()->mDirection;

Let's cache the nsStyleVisibility* result locally rather than looking it up three times.

::: dom/animation/KeyframeEffectReadOnly.cpp:348
(Diff revision 2)
> +  MOZ_ASSERT(false,
> +             "PhysicalizeLogicalProperties does not support for stylo yet");

Can we file a bug for this and refer to it here?

Will this cause existing tests to start crashing when running under stylo if they hit this assertion? If so, we should convert it to a warning instead.

::: dom/animation/KeyframeEffectReadOnly.cpp:370
(Diff revision 2)
>    }
>  
>    nsTArray<AnimationProperty> properties =
>      BuildProperties(Forward<StyleType>(aStyle));
>  
> +  PhysicalizeLogicalProperties(properties, aStyle);

This doesn't seem right. We might now have multiple AnimationProperty objects with the same property, right?

::: dom/base/nsDOMWindowUtils.cpp:2781
(Diff revision 2)
> +    case eStyleAnimType_Logical:
> +      aResult.AssignLiteral("logical");
> +      break;

I don't understand why the need a new animation type, but if we do, we should try to introduce it as a separate patch.
Attachment #8853288 - Flags: review?(bbirtles)
Comment on attachment 8861845 [details]
Bug 1309752 - Part 3: Corresnponds to writing-mode, text-orientation and direction animation for Gecko.

https://reviewboard.mozilla.org/r/133858/#review137188

I've only really skimmed this patch, but I don't understand how it works. Where do we get the (non-animated) writing-mode set on the target element? Where do we get the writing-mode set by independent animations?

::: commit-message-26bae:3
(Diff revision 1)
> +The properties that decide a physical property which is indecated by a logical
> +property can animate discretely. Due to change those properties, physical
> +property might change. In this patch, add a mechanism to animate correct
> +physical property according to changing of writing-mode, text-orientation and
> +direction.

Nit: indicated

Let me check I understand,

"The properties that determine which physical property a logical property corresponds to can be animated (discretely). As a result, the mapping from logical properties specified in keyframes, to the physical properties we animate might change due to animation.

This patch introduces a mechanism to detect changes to these properties (writing-mode, text-orientation, and direction) so that we can update the mapping from keyframes to physical properties, accordingly."

?

::: dom/animation/KeyframeEffectReadOnly.cpp:328
(Diff revision 1)
> -static void
> -PhysicalizeLogicalProperties(nsTArray<AnimationProperty>& aProperties,
> +static void WritingModeStyles(uint8_t& aWritingMode,
> +                              uint8_t& aTextOrientation,
> +                              uint8_t& aDirection,
> -                             nsStyleContext* aStyleContext)
> +                              nsStyleContext* aStyleContext)
>  {
> -  uint8_t writingMode = aStyleContext->StyleVisibility()->mWritingMode;
> -  uint8_t textOrientation = aStyleContext->StyleVisibility()->mTextOrientation;
> -  uint8_t direction = aStyleContext->StyleVisibility()->mDirection;
> +  aWritingMode = aStyleContext->StyleVisibility()->mWritingMode;
> +  aTextOrientation = aStyleContext->StyleVisibility()->mTextOrientation;
> +  aDirection = aStyleContext->StyleVisibility()->mDirection;
> +}
> +
> +static void WritingModeStyles(uint8_t& aWritingMode,
> +                              uint8_t& aTextOrientation,
> +                              uint8_t& aDirection,
> +                              const ServoComputedValuesWithParent& aServoValues)
> +{
> +  MOZ_ASSERT(false,
> +             "WritingModeStyles does not support for stylo yet");
> +}

This feels odd. Perhaps we need a separate struct: WritingModeStyles that we return by value (and perhaps we can change the method added in the last patch to take that as a parameter).

::: dom/animation/KeyframeEffectReadOnly.cpp:361
(Diff revision 1)
> +HoldLogicalProperties(nsTArray<AnimationProperty>& aProperties,
> +                      nsTArray<nsCSSPropertyID>& aLogicalProperties)

GetLogicalProperties?

::: dom/animation/KeyframeEffectReadOnly.cpp:372
(Diff revision 1)
> +static void GetSegmentAt(const AnimationProperty& aProperty,
> +                         const ComputedTiming& aComputedTiming,
> +                         AnimationPropertySegment& aResult)

static void should be on a separate line.

What does GetSegmentAt mean? At what? I guess it means at a progress value? Can we just pass in the progress value instead?

Since this is copying code from KeyframeEffectReadOnly::ComposeStyle why don't we use it there too -- i.e. extract this as a separate patch?

Is there any reason we don't just return the AnimationPropertySegment by value? Or perhaps even a pointer into |aProperty| since the caller presumably owns that?

::: dom/animation/KeyframeEffectReadOnly.cpp:396
(Diff revision 1)
> +void
> +KeyframeEffectReadOnly::WritingModeStyleAt(
> +                          const AnimationProperty& aProp,
> +                          const ComputedTiming& aComputedTiming,
> +                          uint8_t& aResult)

Indentation here is off. It should probably just be two spaces.

::: dom/animation/KeyframeEffectReadOnly.cpp:415
(Diff revision 1)
> +void
> +KeyframeEffectReadOnly::WritingModeStylesAt(
> +                          const ComputedTiming& aComputedTiming,
> +                          uint8_t& aWritingMode,
> +                          uint8_t& aTextOrientation,
> +                          uint8_t& aDirection)

(This too could just return a WritingModeStyle struct assuming we make such an object.)

In general, I wonder if we should be using the name "WritingModeStylesAt". It's a bit confusing because writing-mode is one of the "writing mode styles". Perhaps it should be LogicalPropertyContext?

::: dom/animation/KeyframeEffectReadOnly.cpp:522
(Diff revision 1)
> +  WritingModeStylesAt(computedTiming, currentWritingMode,
> +                      currentTextOrientation, currentDirection);

I don't understand how this works. This seems to be only getting the writing mode etc. from our own animation. What if there are other independent animations changing the writing mode?

::: dom/animation/KeyframeEffectReadOnly.cpp:753
(Diff revision 1)
>                                                result);
>  
>    MOZ_ASSERT(success, "Should be able to extract computed animation value");
>    MOZ_ASSERT(!result.IsNull(), "Should have a valid StyleAnimationValue");
>  
> +  // Special handling for writnig-mode, text-orientation and direction.

writing-mode

::: dom/animation/KeyframeEffectReadOnly.cpp:857
(Diff revision 1)
>  void
>  KeyframeEffectReadOnly::ComposeStyleRule(
>    RefPtr<AnimValuesStyleRule>& aStyleRule,
>    const AnimationProperty& aProperty,
>    const AnimationPropertySegment& aSegment,
>    const ComputedTiming& aComputedTiming)
>  {
> -  StyleAnimationValue fromValue =
> -    CompositeValue(aProperty.mProperty, aStyleRule,
> -                   aSegment.mFromValue.mGecko,
> -                   aSegment.mFromComposite);
> -  StyleAnimationValue toValue =
> +  StyleAnimationValue animationValue;
> +  CalculateAnimationValue(aStyleRule, aProperty, aSegment,
> +                          aComputedTiming, animationValue);
> +
> +  if (animationValue.IsNull()) {
> -    CompositeValue(aProperty.mProperty, aStyleRule,
> -                   aSegment.mToValue.mGecko,
> -                   aSegment.mToComposite);
> -  if (fromValue.IsNull() || toValue.IsNull()) {
>      return;
>    }
>  
>    if (!aStyleRule) {
>      // Allocate the style rule now that we know we have animation data.
>      aStyleRule = new AnimValuesStyleRule();
>    }
> -
> +  aStyleRule->AddValue(aProperty.mProperty, Move(animationValue));
> -  // Iteration composition for accumulate
> -  if (mEffectOptions.mIterationComposite ==
> -      IterationCompositeOperation::Accumulate &&
> -      aComputedTiming.mCurrentIteration > 0) {
> -    const AnimationPropertySegment& lastSegment =
> -      aProperty.mSegments.LastElement();
> -    // FIXME: Bug 1293492: Add a utility function to calculate both of
> -    // below StyleAnimationValues.
> -    StyleAnimationValue lastValue = lastSegment.mToValue.mGecko.IsNull()
> -      ? GetUnderlyingStyle(aProperty.mProperty, aStyleRule)
> -      : lastSegment.mToValue.mGecko;
> -    fromValue =
> -      StyleAnimationValue::Accumulate(aProperty.mProperty,
> -                                      lastValue,
> -                                      Move(fromValue),
> -                                      aComputedTiming.mCurrentIteration);
> -    toValue =
> -      StyleAnimationValue::Accumulate(aProperty.mProperty,
> -                                      lastValue,
> -                                      Move(toValue),
> -                                      aComputedTiming.mCurrentIteration);
> -  }
> -
> -  // Special handling for zero-length segments
> -  if (aSegment.mToKey == aSegment.mFromKey) {
> -    if (aComputedTiming.mProgress.Value() < 0) {
> -      aStyleRule->AddValue(aProperty.mProperty, Move(fromValue));
> -    } else {
> -      aStyleRule->AddValue(aProperty.mProperty, Move(toValue));
> -    }
> -    return;
> -  }
> -
> -  double positionInSegment =
> -    (aComputedTiming.mProgress.Value() - aSegment.mFromKey) /
> -    (aSegment.mToKey - aSegment.mFromKey);
> -  double valuePosition =
> -    ComputedTimingFunction::GetPortion(aSegment.mTimingFunction,
> -                                       positionInSegment,
> -                                       aComputedTiming.mBeforeFlag);
> -
> -  MOZ_ASSERT(IsFinite(valuePosition), "Position value should be finite");
> -
> -  StyleAnimationValue val;
> -  if (StyleAnimationValue::Interpolate(aProperty.mProperty,
> -                                       fromValue,
> -                                       toValue,
> -                                       valuePosition, val)) {
> -    aStyleRule->AddValue(aProperty.mProperty, Move(val));
> -  } else if (valuePosition < 0.5) {
> -    aStyleRule->AddValue(aProperty.mProperty, Move(fromValue));
> -  } else {
> -    aStyleRule->AddValue(aProperty.mProperty, Move(toValue));
> -  }
>  }

There's a lot of code moving around in this patch. If possible, we should try to split off separate patches for extracting functions etc.
Attachment #8861845 - Flags: review?(bbirtles)
Comment on attachment 8861846 [details]
Bug 1309752 - Part 4: Physicalize logical properties at building CSS Transitions.

https://reviewboard.mozilla.org/r/133860/#review137200

Three questions/comments:

1) Don't we need to update the "Stop any transitions..." part of nsTransitionManager::DoUpdateTransitions too? Since we check there if a currently running transition is still in the transition-property and presumably we need to do the logical to physical mapping there too?

2) Is it a problem if we call ConsiderInitiatingTransition twice for the same property? If not, perhaps we could just do the logical -> physical conversion there (or just before calling it) and simplify DoUpdateTransitions? i.e. do we actually need the redundancy checking?

3) If it *is* a problem to call ConsiderInitiatingTransition twice for the same property, then I think we can still simplify the logic here to avoid looping through all the transitions again and again. We could, for example, build up an nsCSSPropertyIDSet in the two branches that need to handle logical properties to represent "transitions on physical properties which we already called ConsiderInitiatingTransition for" (in the 'all' branch we should just fill in the entire set). Then when we go to do transitions for logical properties, just check if the corresponding physical property is in that set or not. We could probably even combine that with the allTransitionProperties set later in this function.

So, in summary, I'm suggesting we do (2) if we can, but (3) if we can't.

If (1) is a problem as I suspect, then I guess we also need more tests to prove to cover that case.

Clearing review request for now, since I'm assuming (1) is a problem we need to fix.

::: layout/style/nsTransitionManager.cpp:660
(Diff revision 1)
> +
> +static inline nsCSSPropertyID
> +PhysicalProperty(nsCSSPropertyID aProperty,
> +                 const ServoComputedValuesWithParent& aComputedStyle)
> +{
> +  MOZ_ASSERT(false, "PhysicalProperty() does not support for Servo yet");

Probably NS_NOTREACHED is better, but do we really want to abort for this? Won't that cause stylo tests that currently pass to crash?

::: layout/style/nsTransitionManager.cpp:666
(Diff revision 1)
> +  return eCSSProperty_UNKNOWN;
> +}
> +
> +
> +static bool
> +HasAnimaition(const nsCSSPropertyID aProperty,

HasAnimation

Or better still: HasAnimationOfProperty

Better yet: HasTransitionOfProperty

(But actually, I wonder if we need this method at all. See comments above.)
Attachment #8861846 - Flags: review?(bbirtles)
Comment on attachment 8861847 [details]
Bug 1309752 - Part 5: Add tests for logical properties animation.

https://reviewboard.mozilla.org/r/133862/#review137506

(This is a large patch. If you could outline the overall approach it takes that would be helpful.)

::: testing/web-platform/tests/web-animations/animation-model/animation-types/addition-per-property.html:20
(Diff revision 1)
> -for (var property in gCSSProperties) {
> -  if (!isSupported(property)) {
> +function testAddition(property, animationTypes, setupFunction,
> +                      logicalProperty, mode) {
> -    continue;

It feels like there should be a more simple way of doing this.

Rather than pass in a 'logicalProperty' bool, can we just do the logical handling inside the loop?

i.e. move the logic you have at the very end of this patch into this loop?

So that if animationType === 'logical', then iterate over the different modes there?

Does that work?

I guess the other problem is that this setup doesn't really let us test combinations of logical and physical?
Comment on attachment 8861847 [details]
Bug 1309752 - Part 5: Add tests for logical properties animation.

https://reviewboard.mozilla.org/r/133862/#review137962

After discussing this and looking into it a little bit further, I think most of the complexity in this patch is warranted. However, I'd still be interested to see if we can move the mode switching into the outer loop in addition-per-property.html and interpolation-per-property.html as mentioned in comment 24.

Also, we'll need to add tests for an independent animation changing the writing-mode.

I'm cancelling review on this due to the above feedback, but also because we're expecting there might be some changes to both the spec (naming) and possible these tests based on the changes suggested to underlying tests. Overall, though, this looks pretty good.
Attachment #8861847 - Flags: review?(bbirtles)
We resolved in the CSS WG to make writing-mode, direction, and unicode-bidi to be not animatable: https://github.com/w3c/csswg-drafts/issues/2751

I think Emilio also had an idea for a simple way of fixing this bug.
Daisuke, is it ok if Emilio steals this bug?
Flags: needinfo?(dakatsuka)
Yes, please!
Flags: needinfo?(dakatsuka)
(Assuming I don't forget about it ;))
Flags: needinfo?(emilio)
Depends on: 1473779
Depends on: 1473793
Attachment #8853287 - Attachment is obsolete: true
Flags: needinfo?(emilio)
Attachment #8853288 - Attachment is obsolete: true
Attachment #8861845 - Attachment is obsolete: true
Attachment #8861846 - Attachment is obsolete: true
Attachment #8861847 - Attachment is obsolete: true
Assignee: dakatsuka → emilio
This most notably needs tests. There's a bunch of tests to update / write in test_transitions_per_property.html and such. I can also write tests for @keyframes and such without too much trouble.

Brian / Hiro, is there any chance any of you could write tests for web animations? It'd take you half the time than what it'd take me if not less.

Also, some things that are probably worth checking / I haven't tested:

 * Keyframes with both logical properties and physical ones that conflict. I suspect I need to iterate something in reverse, I've left a FIXME there, but I haven't tested. I'll take a look tomorrow if you don't win me at it.
 * General web animation sanity. I've checked that web animations properly work even if the keyframes specify different logical or physical properties, but Brian scared me last week re. a bunch of complexity regarding shorthands I didn't really parse.
Flags: needinfo?(hikezoe)
Flags: needinfo?(bbirtles)
I am afraid I can't time the take for it soon, but I think Boris can do that as of today.  CCing him.
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> I am afraid I can't time the take for it soon, but I think Boris can do that
> as of today.  CCing him.

Oh, sure, it doesn't _need_ to be today, just sometime soon :).

I'm going to be looking at bug 1471583 & co. today. But thanks!
I've gone through these patches and they look really great. Building locally now to see if I can test some of those edge cases.
Flags: needinfo?(bbirtles)
Comment on attachment 8990590 [details]
Bug 1309752: Add a method to physicalize longhands.

https://reviewboard.mozilla.org/r/255650/#review262392

::: layout/style/nsCSSProps.h:158
(Diff revision 1)
>      MOZ_ASSERT(0 <= aProperty && aProperty < eCSSProperty_COUNT,
>                 "out of range");
>      return (nsCSSProps::kFlagsTable[aProperty] & aFlags) == aFlags;
>    }
>  
> +  static inline nsCSSPropertyID Physicalize(nsCSSPropertyID aProperty,

Nit: no "inline" needed.
Attachment #8990590 - Flags: review?(cam) → review+
Comment on attachment 8990591 [details]
Bug 1309752: Introduce PropertyDeclaration::to_physical.

https://reviewboard.mozilla.org/r/255652/#review262394

::: servo/components/style/properties/properties.mako.rs:1959
(Diff revision 1)
> +        unsafe {
> +            let longhand_id = *(&mut ret as *mut _ as *mut LonghandId);
> +            debug_assert_eq!(PropertyDeclarationId::Longhand(longhand_id), ret.id());
> +
> +            *(&mut ret as *mut _ as *mut LonghandId) = longhand_id.to_physical(wm)
> +        }

Can you add a short comment similar to the one in PropertyDeclaration::id?
Attachment #8990591 - Flags: review?(cam) → review+
Attached patch Web Animations tests (obsolete) — — Splinter Review 
Here are the tests for the Web Animations API.

Most of them pass but I think we need to add something to make physical properties beat logical properties in Web Animations keyframes.

Or is that even the right behavior? In Web Animations we have this logic for making sure longhand properties override shorthands (and more specific shorthands override more general ones) since we can rely on declaration order for the properties of JS objects like we can with CSS declaration blocks.

That logic is specified here:

  https://drafts.csswg.org/web-animations-1/#calculating-computed-keyframes

Scroll down to the part starting, "If conflicts arise when expanding shorthand properties due to shorthand properties overlapping with existing longhand properties..."

Based on that, I figured that physical properties are more specific than logical ones so if both are specified on a keyframe the physical ones should win. Perhaps I should add that to the Web Animations spec as part of the steps mentioned above.
Attached patch Web Animations tests v2 (obsolete) — — Splinter Review 
(Updated based so I can build on it more easily.)
Attachment #8990638 - Attachment is obsolete: true
(Applies on top of the Web Animations tests)
Comment on attachment 8990592 [details]
Bug 1309752: Animate logical properties.

https://reviewboard.mozilla.org/r/255654/#review262390

As part of reviewing this I prepared attachment 8990648 [details] [diff] [review] but it seems to reveal a bug here.

Specifically, if we have the following animation:

@keyframes anim {
  from { block-size: 0px; height: 200px }
  to   { block-size: 100px; height: 300px }
}

My understanding based on Example 2 in the spec[1] is that since these share a computed value, and since the 'height' declaration comes later, we should end up animating from 200px to 250px but instead we seem to animate from 0px to 100px.

I'm pretty sure this corresponds to the FIXME in Servo_StyleSet_GetKeyframesForName.

Are you going to fix that in this patch or in a separate one?

[1] https://drafts.csswg.org/css-logical/#box

::: servo/components/style/properties/helpers/animated_properties.mako.rs:260
(Diff revision 1)
> -    % if prop.animatable:
> +    % if prop.animatable and not prop.logical:
>      /// `${prop.name}`
>      ${prop.camel_case}(${prop.animated_type()}),
>      % else:
>      /// `${prop.name}` (not animatable)

(Nit: generating a comment like "/// `margin-inline-start` (not animatable)" is not strictly correct...)
Attached patch Web Animations tests v3 (obsolete) — — Splinter Review 
Fixed a bug in the direction change test
Attachment #8990647 - Attachment is obsolete: true
Rebase because MANIFEST.json...
Attachment #8990648 - Attachment is obsolete: true
Comment on attachment 8990593 [details]
Bug 1309752: Update animations if the logical to physical property changes.

https://reviewboard.mozilla.org/r/255656/#review262408

::: servo/components/style/matching.rs:291
(Diff revision 1)
> +        }
> +
> +        // We might need to update animations if writing-mode or direction
> +        // changed, and any of the animations contained logical properties.
> +        //
> +        // We may want to be more granular, but it's probably not worth.

(Nit: s/not worth/not worth it)
Attachment #8990593 - Flags: review?(bbirtles) → review+
Comment on attachment 8990594 [details]
Bug 1309752: Cleanup might_need_transitions_update.

https://reviewboard.mozilla.org/r/255658/#review262410

Obviously we'll need to add wpt tests for transitions but I can probably help with that.
Attachment #8990594 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #44)
> Comment on attachment 8990592 [details]
> Bug 1309752: Animate logical properties.
> 
> https://reviewboard.mozilla.org/r/255654/#review262390
> 
> As part of reviewing this I prepared attachment 8990648 [details] [diff] [review]
> [review] but it seems to reveal a bug here.
> 
> Specifically, if we have the following animation:
> 
> @keyframes anim {
>   from { block-size: 0px; height: 200px }
>   to   { block-size: 100px; height: 300px }
> }
> 
> My understanding based on Example 2 in the spec[1] is that since these share
> a computed value, and since the 'height' declaration comes later, we should
> end up animating from 200px to 250px but instead we seem to animate from 0px
> to 100px.
> 
> I'm pretty sure this corresponds to the FIXME in
> Servo_StyleSet_GetKeyframesForName.

Yup, indeed :)

> Are you going to fix that in this patch or in a separate one?

I'll fix this in this patch, not a big issue.

> [1] https://drafts.csswg.org/css-logical/#box
> 
> ::: servo/components/style/properties/helpers/animated_properties.mako.rs:260
> (Diff revision 1)
> > -    % if prop.animatable:
> > +    % if prop.animatable and not prop.logical:
> >      /// `${prop.name}`
> >      ${prop.camel_case}(${prop.animated_type()}),
> >      % else:
> >      /// `${prop.name}` (not animatable)
> 
> (Nit: generating a comment like "/// `margin-inline-start` (not animatable)"
> is not strictly correct...)

Nice catch :)

(In reply to Brian Birtles (:birtles) from comment #41)
> Created attachment 8990638 [details] [diff] [review]
> Web Animations tests
> 
> Here are the tests for the Web Animations API.
> 
> Most of them pass but I think we need to add something to make physical
> properties beat logical properties in Web Animations keyframes.
> 
> Or is that even the right behavior? In Web Animations we have this logic for
> making sure longhand properties override shorthands (and more specific
> shorthands override more general ones) since we can rely on declaration
> order for the properties of JS objects like we can with CSS declaration
> blocks.
> 
> That logic is specified here:
> 
>   https://drafts.csswg.org/web-animations-1/#calculating-computed-keyframes
> 
> Scroll down to the part starting, "If conflicts arise when expanding
> shorthand properties due to shorthand properties overlapping with existing
> longhand properties..."
> 
> Based on that, I figured that physical properties are more specific than
> logical ones so if both are specified on a keyframe the physical ones should
> win. Perhaps I should add that to the Web Animations spec as part of the
> steps mentioned above.

That reasoning makes sense, but probably should be explicit from the spec. Right now we sort longhands and custom properties as having the same order. To implement this I need to decide an order between the three (physical longhand, logical longhand, custom). We've already decided that physical longhand > logical longhand, so we just need to decide at which side custom properties fall. We could also keep sorting them at the same priority as physical longhands, though that'd be somewhat weird. I guess the order between custom properties and longhands doesn't really matter, so... Any preference? :)
Flags: needinfo?(bbirtles)
Comment on attachment 8990590 [details]
Bug 1309752: Add a method to physicalize longhands.

https://reviewboard.mozilla.org/r/255650/#review262558

::: layout/style/nsCSSProps.h:158
(Diff revision 1)
>      MOZ_ASSERT(0 <= aProperty && aProperty < eCSSProperty_COUNT,
>                 "out of range");
>      return (nsCSSProps::kFlagsTable[aProperty] & aFlags) == aFlags;
>    }
>  
> +  static inline nsCSSPropertyID Physicalize(nsCSSPropertyID aProperty,

Thanks, copy pasta :)

I'll fix the others as well.
Comment on attachment 8990591 [details]
Bug 1309752: Introduce PropertyDeclaration::to_physical.

https://reviewboard.mozilla.org/r/255652/#review262394

> Can you add a short comment similar to the one in PropertyDeclaration::id?

Yes, also fixed that one, which mentioned PropertyDeclarationId instead of PropertyDeclaration.
> (In reply to Brian Birtles (:birtles) from comment #41)
> > Created attachment 8990638 [details] [diff] [review]
> > Web Animations tests
> > 
> > Here are the tests for the Web Animations API.
> > 
> > Most of them pass but I think we need to add something to make physical
> > properties beat logical properties in Web Animations keyframes.
> > 
> > Or is that even the right behavior? In Web Animations we have this logic for
> > making sure longhand properties override shorthands (and more specific
> > shorthands override more general ones) since we can rely on declaration
> > order for the properties of JS objects like we can with CSS declaration
> > blocks.
> > 
> > That logic is specified here:
> > 
> >   https://drafts.csswg.org/web-animations-1/#calculating-computed-keyframes
> > 
> > Scroll down to the part starting, "If conflicts arise when expanding
> > shorthand properties due to shorthand properties overlapping with existing
> > longhand properties..."
> > 
> > Based on that, I figured that physical properties are more specific than
> > logical ones so if both are specified on a keyframe the physical ones should
> > win. Perhaps I should add that to the Web Animations spec as part of the
> > steps mentioned above.
> 
> That reasoning makes sense, but probably should be explicit from the spec.
> Right now we sort longhands and custom properties as having the same order.
> To implement this I need to decide an order between the three (physical
> longhand, logical longhand, custom). We've already decided that physical
> longhand > logical longhand, so we just need to decide at which side custom
> properties fall. We could also keep sorting them at the same priority as
> physical longhands, though that'd be somewhat weird. I guess the order
> between custom properties and longhands doesn't really matter, so... Any
> preference? :)

I don't think it matters since they don't animate the same computed value.

I'll work on the Web Animations spec changes and try to finish up the CSS animations tests today. Are you able to take care of the CSS transitions' tests or should I put that on my list?
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #57)
> I don't think it matters since they don't animate the same computed value.
> 
> I'll work on the Web Animations spec changes and try to finish up the CSS
> animations tests today. Are you able to take care of the CSS transitions'
> tests or should I put that on my list?

I updated the patch with an arbitrary order last night. I can look into those tests yeah :)
Attached patch Animation tests (obsolete) — — Splinter Review 
Dropped the tests/comments around text-orientation, fixed a few typos, and merged these tests into one patch.
Attachment #8990650 - Attachment is obsolete: true
Attachment #8990651 - Attachment is obsolete: true
Comment on attachment 8990592 [details]
Bug 1309752: Animate logical properties.

https://reviewboard.mozilla.org/r/255654/#review262726

::: servo/components/style/properties/helpers/animated_properties.mako.rs:260
(Diff revision 2)
> -    % if prop.animatable:
> +    % if prop.animatable and not prop.logical:
>      /// `${prop.name}`
>      ${prop.camel_case}(${prop.animated_type()}),
>      % else:
>      /// `${prop.name}` (not animatable)
>      ${prop.camel_case}(Void),

(Not sure if you were planning on fixing this or not.)

::: servo/ports/geckolib/glue.rs:4929
(Diff revision 2)
>                  // Filter out non-animatable properties and properties with
>                  // !important.
> -                for declaration in guard.normal_declaration_iter() {
> +                for declaration in guard.normal_declaration_iter().rev() {

Does this need a comment to say why we iterate in reverse?
Attachment #8990592 - Flags: review?(bbirtles) → review+
Comment on attachment 8990883 [details] [diff] [review]
Tests for Web Animations and CSS Animations

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

They look amazing, thanks!

::: testing/web-platform/tests/css/css-logical/animation-002.html
@@ +15,5 @@
> +  addStyle(t, {
> +    '@keyframes anim': 'from { block-size: 0px } to { block-size: 100px }',
> +  });
> +  const div = addDiv(t, { style: 'animation: anim 10s -5s paused linear' });
> +  assert_equals(getComputedStyle(div).height, '50px');

This is so much easier than the way I'd have tested this.

::: testing/web-platform/tests/css/css-logical/support/testcommon.js
@@ +1,4 @@
> +/*
> + Distributed under both the W3C Test Suite License [1] and the W3C
> + 3-clause BSD License [2]. To contribute to a W3C Test Suite, see the
> + policies and contribution forms [3].

I haven't reviewed this assuming that it's the same file as used elsewhere, please let me know if I should.
Attachment #8990883 - Flags: review?(emilio) → review+

        Attachment #8992639 -
        Flags: review?(bbirtles)

    
    

    
  
Comment on attachment 8992639 [details] [diff] [review]
Transitions tests.

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

We should add WPT for this too that more specifically tests some of the overlapping cases.

I'm happy to help with that if you like. For now, let's land this. We can add that test later.

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

    
    

    
  
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c21cb4fc9994
Add a method to physicalize longhands. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5fa7f73f549
Introduce PropertyDeclaration::to_physical. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/851648d9e971
Animate logical properties. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4bd0fbb7865
Update animations if the logical to physical property mapping changes. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba634ab98bd
Cleanup might_need_transitions_update. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9afe9ab2989
Transition tests. r=birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/1467f56b0eee
Tests for Web Animations and CSS Animations of logical longhands. r=emilio

    
    

    
  
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12050 for changes under testing/web-platform/tests

    
    

    
  
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

    
    

    
  
Note for MDN folks: We need to update the animation type for the relevant logical properties to match their physical counterparts.
Depends on: 1476927

    
    

    
  
I've updated the animation type data for the logical properties as appropriate:

https://github.com/mdn/data/pull/282

And added a note to the Fx63 rel notes:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63#CSS

As a note, things I haven't added are as follows (not implemented, beyond the scope of this bug):

* The flow-relative directional keyword values for top/bottom/left/right
* The inline-start/inline-end values for the caption-side/float/clear property
* The margin-block/margin-inline shorthands are not implemented yet, so not added to the data
* The inset-block/inset-inline/inset shorthands are not implemented yet, so not added to the data
* The padding-block/padding-inline shorthands are not implemented yet, so not added to the data
* The border-block-width/ border-inline-width/ border-block-style/ border-inline-style/ border-block-style/ border-inline-style/ border-block-color/ border-inline-color shorthands are not implemented yet, so not added to the data
* The border-block/border-inline shorthands are not implemented yet, so not added to the data
* The border-start-start-radius/ border-start-end-radius/ border-end-start-radius/ border-end-end-radius properties are not implemented yet, so not added to the data
* The "logical" keyword is not implemented yet, so not added to the data 

Let me know if that's OK for now. Thanks!
Flags: needinfo?(emilio)
Keywords: dev-doc-needed → dev-doc-complete

    
    

    
  
That seems right to me. Thanks Chris!

    
    

    
  
Yeah, that's great, thanks a lot Chris :)
Flags: needinfo?(emilio)

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: