1551818 - Web animations mishandle aliases with CSS wide keywords

Status: NEW

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

Description

[Emilio Cobos Álvarez (:emilio)]

If you add -webkit-appearance to the test here:

https://searchfox.org/mozilla-central/rev/116bd975c30746ddefc3d20e6947d1871469354f/dom/animation/test/mozilla/test_moz_prefixed_properties.html#17

It fails. This bit me when trying to unprefix user-select.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I haven't looked much into this, but I believe it's unexpected that aliases work differently than the properties they alias to.

Let me know if you don't plan / don't have the cycles to dig into this and I'll do.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

It's not particularly related to wide keywords:

animation = document.body.animate({ "transform": ["translate(0)", "translate(100px)" ] }, { duration: 1000 });

Works, while:

animation = document.body.animate({ "MozTransform": ["translate(0)", "translate(100px)" ] }, { duration: 1000 });

Doesn't

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I'll take a look after lunch.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1551818 - Make IDL name lookup work for aliases. r=birtles

This seems it's only used by web animations.

Comment 5

[Brian Birtles (:birtles)]

Emilio, I left some comments in the Phabricator review yesterday, not sure if you saw them. I'm currently just trying to make sure aliases for shorthands work correctly (e.g. -moz-columns? -webkit-flex? -webkit-mask?) but keep running out of disk space when I go to build.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I did see them, but was pretty busy today trying to get my transform patches landed so that Boris could rebase on top and such. I'll get back to this tomorrow or on monday.

Comment 7

[Brian Birtles (:birtles)]

Ok great, just making sure you weren't waiting on me to finish reviewing.

Comment 8

[Brian Birtles (:birtles)]

So I dug into this a bit because I wasn't sure how this was supposed to work.

In the past Web Animations had special handling for aliases along the following lines:

1.  For user agents that support both a prefixed and an
    unprefixed version of some CSS properties, remove all
    prefixed properties from <var>animation properties</var>
    where the corresponding unprefixed version is also
    present in <var>animation properties</var>.

(i.e. the idea being to only read aliased properties if they weren't overlapped by anything else.)

This was dropped in: https://github.com/w3c/web-animations/commit/32413328b04d5ac4109b7424df00834de20a66ba#diff-8d4d847e6257b75f4bf8030496281de4L7468

The replacement text being added a few commits prior in: https://github.com/w3c/web-animations/commit/90e5a5319884892f51c26c293b5fc11c5c89094a#diff-8d4d847e6257b75f4bf8030496281de4R6922

I recall that at that time Google were pretty keen that Web Animations should not support prefixed properties at all. I persuaded them we had to have at least some support in order to represent CSS animations/transitions of properties for which there was no unprefixed version available.

That whole discussion happened before we started to specify these properties however.

So the current behavior is that we simply don't support aliased properties. That appears to be consistent across Chrome Canary, Safari TP, and Firefox at least for 'WebkitTransform': https://codepen.io/birtles/pen/byWmQx

But given that prefixed properties may be unprefixed by aliasing, I guess you're suggesting that we really should handle aliased properties (otherwise we would break any content using the prefixed version). That makes sense to me.

Given that, if you specify:

const anim = elem.animate({ MozTransform: 'translate(100px)' }, 1000);
console.log(anim.effect.getKeyframes());

What should you get?

1. { MozTransform: 'translate(100px)' }? i.e. alias resolution happens later
2. { transform: 'translate(100px)' }? i.e. alias resolution happens on input
3. { MozTransform: 'translate(100px)', transform: 'translate(100px)' }? both are returned?

Furthermore, what happens when you specify both, e.g.:

{ MozTransform: 'translate(100px)', transform: 'translate(200px)' }

Presumably transform should win in that case (i.e. aliases lose to whatever they alias).

Of the options, I think we definitely don't want (3). It would be an authoring nightmare.

For similar reasons (2) is also pretty bad. e.g. the following code won't work:

const anim = elem.animate({ MozTransform: 'translate(100px)' }, 1000);
// Update target translation to 200px
anim.effect.setKeyframes({ ...anim.effect.getKeyframes()[0], MozTransform: 'translate(200px)' });

In that case we'd end up returning { transform: 'translate(100px)' }, to which the code appends MozTransform but that gets ignored when fed back in.

So it sounds like we need to change the spec to preserve aliases, i.e. (1).

That would mostly involve updating the section where we calculate computed keyframes to include rules for resolving aliases (like we already have to logical properties etc.)

If that makes sense to you let me know and I'll file a spec issue for this.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I think we should get 2. Right now alias resolution happens as soon as we parse the property basically.

Comment 10

[Brian Birtles (:birtles)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

I think we should get 2. Right now alias resolution happens as soon as we parse the property basically.

With these patches we'll actually crash for this test case (due to assertions in PropertyIDLName and PropertyIDLNameSortPosition that that need to be updated).

However, with those fixed we'll get behavior 1. That's because we store the nsCSSPropertyID in addition to the RawServoDeclarationBlock.

These patches will still need extra work, however, to ensure we that when both MozTransform and transform are specified transform wins. Test case: https://codepen.io/birtles/pen/xNLWvr

Let me know if you agree we want behavior (1) and go I'll go ahead and file the spec issue / PR.

Comment 11

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #10)

Let me know if you agree we want behavior (1) and go I'll go ahead and file the spec issue / PR.

Filed https://github.com/w3c/csswg-drafts/issues/3948 for this

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:birtles, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D31250	Bug 1551818 - Make IDL name lookup work for aliases. r=birtles	emilio	birtles: Inactive

:emilio, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.