1266570 - "clip-path" shapes don't transition between percent and pixel coordinates

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Spinning this off from bug 1110460 comment 64 - 65.  I'll attach a testcase shortly.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

In this testcase, the lower circle doesn't transition smoothly, but the upper one does.

So, we seem to be able to transition between percent vs. pixels for the center, but not for the radius.

Comment 2

[Daniel Holbert [:dholbert]]

(Note: to test this bug, you need to be sure you have layout.css.clip-path-shapes.enabled = true in about:config.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (transitioning circle position vs. radius)

(Here's a revised "testcase 1" which includes the prefixed version of this property, so that it can be tested in Chrome.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: transitioning a polygon's point

Comment 5

[Daniel Holbert [:dholbert]]

The proximal issue here is:
 * We use GetCommonUnit() when doing the interpolation of the radius & the polygon points here.
 * That function checks the CSS_PROPERTY_STORES_CALC flag when deciding whether to allow percent/pixel interpolation (and represents the intermediate value using a "calc()" expression.
 * This property, "clip-path", does not have the CSS_PROPERTY_STORES_CALC flag, so GetCommonUnit just fails.

(We succeed at interpolating the circle's center-position, though, because that's stored as a CSS <position> which is explicitly calc()-ified under the hood, and interpolated using "AddPositions".)

A trivial fix is to simply add the CSS_PROPERTY_STORES_CALC flag for this property, though I'm not sure offhand if that has any undesirable effects/implications.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: strawman fix v1

(This fixes the bug with both attached testcases, though I'm not sure it's correct, per end of previous comment.)

Comment 7

[Daniel Holbert [:dholbert]]

With my attached "strawman fix", we start spitting out values like the following:
   "polygon(calc(67.805px + 5.47967%) 10%, 0% 100%, 100% 100%)"
...as the computed value of "clip-path" (if queried via getComputedStyle().

However, we don't currently accept such values in the CSS parser -- they don't roundtrip correctly, if the author were to provide something like that ^^ as a specified value. (using "calc")

Chrome also outputs values like this from getComputedStyle (for -webkit-clip-path), and it *does* accept them in the CSS parser. Chrome is correct -- https://drafts.csswg.org/css-shapes/#supported-basic-shapes says that these arguments are of type <length>, which means calc() should be accepted.  (Later on in that spec-text, it explicitly discusses calc() as a possibility, too.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 3: static test for calc() support

Hmm, actually it looks like we do support calc() inside these properties, according to this testcase (in which case, the strawman fix is actually correct).

Let me see if I can figure out why I thought we didn't...

Comment 9

[Daniel Holbert [:dholbert]]

(Yeah, I must've just mis-pasted in the calc()-involved expression. Looks like calc() works just fine in this property. So, I believe the strawman patch is all we need, along with tests.)

Comment 10

[Daniel Holbert [:dholbert]]

(I can't actually reliably add tests for this right now, since the existing "clip-path" tests aren't runnable yet, per bug 1266868.  One that bug's fixed, I'll add some new tests here and we can get this bug fixed.)

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> (I can't actually reliably add tests for this right now, since the existing
> "clip-path" tests aren't runnable yet, per bug 1266868.

(I spun off some of the issues that prevent these tests from running -- bug 1269990 and bug 1269992.)

Comment 12

[Daniel Holbert [:dholbert]]

Looks like I should be able to add tests now and get this landed, since the existing tests can be run successfully now (when the pref is enabled) according to bug 1297333.

I'll hopefully get to closing this out early next week.

Comment 13

[Daniel Holbert [:dholbert]]

(er sorry, s/landed/reviewed/ I guess.  :) I misremembered that this had been reviewed back when I was first looking & got blocked by the test issues.)

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: Bug 1266570: Add CSS_PROPERTY_STORES_CALC flag to "clip-path" property, so it can transition between percent and pixel shape-coordinates.

Our transition/animation code uses "calc" to represent interpolated values between percent & pixel endpoints.  But it only does that if the property has this CSS_PROPERTY_STORES_CALC flag. (This check is in StyleAnimationValue.cpp's static helper, GetCommonUnit().)

We do indeed store (& honor) calc() units for "clip-path" in its style struct, so we can & should add this flag.

Review commit: https://reviewboard.mozilla.org/r/79798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79798/

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8792987 [details]
Bug 1266570: Add CSS_PROPERTY_STORES_CALC flag to "clip-path" property, so it can transition between percent and pixel shape-coordinates.

https://reviewboard.mozilla.org/r/79798/#review78546

::: layout/style/test/test_transitions_per_property.html:693
(Diff revision 1)
>      expected: ["polygon", ["evenodd, 200% 200%, 200% 200%"], "border-box"]
>    },
>    { start: "inset(100% 100% 100% 100% round 100% 100%) border-box",
>      end: "inset(500% 500% 500% 500% round 500% 500%) border-box",
>      expected: ["inset", ["200% round 200%"], "border-box"] },
> +  // matching functions with calc() values

Note: the first three new testcases (the ones with "calc" in the start/end values) **already pass**, without the actual code-change here.  That proves that we do indeed already honor & store calc() in the style structs for this property -- because we're matching the expected endpoint with its interpolated calc() value.

This bug's one-liner fix makes that calc() storage official, so that our StyleAnimation code knows it's OK to generate intermediate values that use calc() when given pixel values & percent values to interpolate between. And that makes the final two new testcases pass.

Comment 16

[Daniel Holbert [:dholbert]]

Try run (with helper patch stolen from bug 1297333, to enable this feature for testing):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad7f6d59afc5

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8792987 [details]
Bug 1266570: Add CSS_PROPERTY_STORES_CALC flag to "clip-path" property, so it can transition between percent and pixel shape-coordinates.

https://reviewboard.mozilla.org/r/79798/#review78702

Looks good, thanks.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1da8daaf2082
Add CSS_PROPERTY_STORES_CALC flag to "clip-path" property, so it can transition between percent and pixel shape-coordinates. r=heycam

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1da8daaf2082

Comment 20

[Chris Mills [:cmills]]

I've checked to make sure that an accurate note reflecting this bug has been added to the Firefox 52 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/52#CSS

Is this correct? The problem has been fixed now, right? The test cases seemed to work fine in the latest nightly.

Also, I've not added anything about this to the transition or clip-path reference pages - I didn't think it was necessary if the problem has been fixed, and it is such a nascent feature anyway.

Comment 21

[Daniel Holbert [:dholbert]]

Thanks Chris! I'm not sure it makes sense to note this in release notes, since this is part of a feature that's disabled by default in Release & Beta:
https://dxr.mozilla.org/mozilla-central/rev/3cedab21a7e65e6a1c4c2294ecfb5502575a46e3/modules/libpref/init/all.js#2500-2505
...but it's probably not a big deal either way.

Agreed that we don't need to mention anything in particular about this in the transition/clip-path pages. This was simply a bug, which is now fixed, which doesn't need any special explanation/documentation.

Comment 22

[Chris Mills [:cmills]]

(In reply to Daniel Holbert [:dholbert] from comment #21)
> Thanks Chris! I'm not sure it makes sense to note this in release notes,
> since this is part of a feature that's disabled by default in Release & Beta:
> https://dxr.mozilla.org/mozilla-central/rev/
> 3cedab21a7e65e6a1c4c2294ecfb5502575a46e3/modules/libpref/init/all.js#2500-
> 2505
> ...but it's probably not a big deal either way.

Ah, damn! That is a good point that I didn't clock (the perils of always using Nightly); in this case it shouldn't be mentioned in the release notes either, so I've deleted the note ;-)

We'll mention it when it is activated in beta/release.