1424939 - The shapes path tool shouldn't follow animated elements

Status: NEW

(DevTools :: Inspector, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

Steps:

- load this URL in a tab: data:text/html,<style>div {width:300px;height:300px;background:red;animation:a 3s alternate infinite;}@keyframes a {from {clip-path:polygon(0 0, 50% 0, 100% 50%, 50% 100%, 0 100%);} to {clip-path:polygon(0 0, 100% 0, 100% 50%, 100% 100%, 0 100%);}}</style><div>
- open the inspector and select the <div>
- in the rule-view, you should see the @keyframes rule, with the clip-path property being animated
- click on the shapes editor icon in there and try and drag the points around

--> Nothing happens, you can't drag points around
And the following error is displayed in the browser console:

TypeError: this.coordUnits[point] is undefined  shapes.js:813:9

Now, there are several issues here:
- if the shape is moving, it's hard to grab the points, but I think this is another bug altogether. You can still use the animation panel to pause the animation. Another idea is that perhaps the shape should not auto-refresh and follow the shape. If you click on the icon in the 0% rule, then that particular shape should be highlighted, not the shape it currently is.

- if dragging a point worked, then we'd have to make the edit right inside the actual source rule (the 0% or 100%), not override it into a new inline style like we do today, cause that wouldn't impact the animation at all.

Comment 1

[Patrick Brosset <:pbro>]

This is an important one to get in by Firefox 60 because that's when we plan to ship CSS shapes in Firefox and announce the feature and the tool together.

Comment 2

[Patrick Brosset <:pbro>]

The best thing to do here for now is probably to disable the shapes icon inside of @keyframes rule.

Comment 3

[Astley Chen (inactive)]

(In reply to Patrick Brosset <:pbro> (away for xmas until Jan. 3rd) from comment #2)
> The best thing to do here for now is probably to disable the shapes icon
> inside of @keyframes rule.

That seems a sensible solution for now.

Comment 4

[Patrick Brosset <:pbro>]

I think the way to implement comment 2 is the following:

1. OutputParser.js is what we use to parse a CSS property value and "augment" it with icons and such, like the shapes icon in case of clip-path or shape-outside.
This is where the code decides whether to do it or not: https://searchfox.org/mozilla-central/rev/51cd1093c5c2113e7b041e8cc5c9bf2186abda13/devtools/client/shared/output-parser.js#93
As you can see, it is already based on an option, but this option can not be set yet. It only takes it value on whether the CSS property value is a shape or not.
I would advise to first check whether options.expectShape exists and contains a value. If expectShape is already defined and is false, then we should use this false value instead of checking for clip-path or shape-outside.

2. With this in place, we would need to set this option to false at the right place.
I believe this place is here: https://searchfox.org/mozilla-central/rev/51cd1093c5c2113e7b041e8cc5c9bf2186abda13/devtools/client/inspector/rules/views/text-property-editor.js#166
For now, we do not pass any options here. I suggest that we start passing { expectShape: false } right here, as a third parameter to parseCssProperty, but only when the rule we are currently populating is a @keyframes rule.
This can be done by accessing the object this.ruleEditor.rule which, I think, should contain some information about whether the rule is whithin a @keyframes rule.

Comment 5

[Patrick Brosset <:pbro>]

Bug 1435373 fixes the second problem described in comment 0. So let's just depend on it.

And Let's re-purpose this bug for the first problem only: while the element is being animated, the shape editor tool animates with it. This is making editing points impossible. And should really not happen.
Why? Because if you're updating the clip-path property of one of the animation keyframe rules, you're editing the shape at that particular point in time, not throughout the animation's duration.
So I believe the tool should appear at the position that his keyframe defines, and not move.