1074522 - Implement ellipse/circle shape functions for clip-path

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Dirk Schulze]

Attachment: clip-path-ellipse.patch

This patch implements the ellipse() and circle() basic-shape functions from CSS Shapes for the 'clip-path' property.

http://dev.w3.org/csswg/css-shapes/#basic-shape-functions
http://dev.w3.org/fxtf/css-masking-1/#the-clip-path

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is additional code needed to support the values?  (We shouldn't parse them until we support them.)  Or am I missing something in here that makes them "just work"?

What's the status of this relative to https://wiki.mozilla.org/WebAPI/ExposureGuidelines ?  i.e., how stable is the spec, and what are other browsers doing?

Comment 3

[Dirk Schulze]

I sent an intent to implement to dev-platform for clip-path features. Everything is behind a flag. So there is no damage in starting with the parsing first. The specification (CSS Masking) is in CR and stable. What I am implementing here is already supported by the -webkit-clip-path property in Safari and hopefully will be unprefixed with the next release next year after fixing a couple of bugs.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Dirk Schulze from comment #3)
> I sent an intent to implement to dev-platform for clip-path features.
> Everything is behind a flag. So there is no damage in starting with the
> parsing first.

But bug 1057180 turns that flag on, which means there is.

> The specification (CSS Masking) is in CR and stable.

OK, that settles the stability bit. :-)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8497143 [details] [diff] [review]
clip-path-ellipse.patch

>+      "circle(",
...
>+      "polygon(",

If you want to test these, they should be in unbalanced_values
rather than invalid_values.  See the comment at the top of
property_database.js.


nsCSSParser.cpp:

>+CSSParserImpl::ParseEllipticFunctions

Maybe just call this ParseCircleOrEllipse?


>+        nsCSSKeywords::LookupKeyword(mToken.mIdent) !=
>+        eCSSKeyword_at) {

Just use mToken.mIdent.LowerCaseEqualsLiteral("at") and skip the
addition of the new token.


This is rather an odd way to use eCSSUnit_Function; I think I'd
prefer if you actually used 2 arguments for a circle and 3 for an
ellipse.  This would also force you to think about what value
to use for "unspecified" more clearly, which would avoid the weird
single-null-value list that you currently produce for the function
with no arguments.

>+  if (!ExpectSymbol(')', true)) {
>+
>+    if (mToken.mType != eCSSToken_Ident ||
>+        nsCSSKeywords::LookupKeyword(mToken.mIdent) !=
>+        eCSSKeyword_at) {

I think it matches the expected logic more closely if you have the check
for the 'at' keyword first, and put the indented section of the function
(except for the ExpectSymbol(')'), which would move out) inside a test
for 'at' rather than a test for the failure of ExpectSymbol.

I also don't know how you even succeed at parsing any of the position
values, since ExpectSymbol calls UngetToken when it fails, so I don't
see anything that ever consumes the 'at' token.  Did you actually run
the tests with the pref enabled?

nsCSSProps:

>kEllipseRadiusKTable

Call this kShapeRadiusKTable to match the spec's term.

nsCSSValue:

nsCSSValue::AppendEllipseToString seems like it will mess up
serializing 'circle()' or 'ellipse()' because of that null value
I mentioned above.  (Did you run the tests with the pref on?)

It also seems like the repeated code for the 2 radii could be reduced,
or put in a function.


nsStyleBasicShape should have comments explaining how the radii are stored.


I didn't look at the nsComputedDOMStyle code or nsRuleNode code yet,
but marking review- based on the above.

Comment 6

[Dirk Schulze]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #4)
> (In reply to Dirk Schulze from comment #3)
> > I sent an intent to implement to dev-platform for clip-path features.
> > Everything is behind a flag. So there is no damage in starting with the
> > parsing first.
> 
> But bug 1057180 turns that flag on, which means there is.
> 

I use a different flag for clip-path called layout.css.clip-path-shapes. I'll send an intent to ship after the implementation is complete.

Comment 7

[Dirk Schulze]

Attachment: clip-path-ellipse.patch

Comment 8

[Dirk Schulze]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #5)
> Comment on attachment 8497143 [details] [diff] [review]
> clip-path-ellipse.patch
> 
> >+      "circle(",
> ...
> >+      "polygon(",
> 
> If you want to test these, they should be in unbalanced_values
> rather than invalid_values.  See the comment at the top of
> property_database.js.

Thanks. I changed it and added these kind of tests to unbalanced_values.

> 
> 
> nsCSSParser.cpp:
> 
> >+CSSParserImpl::ParseEllipticFunctions
> 
> Maybe just call this ParseCircleOrEllipse?
> 
> 
> >+        nsCSSKeywords::LookupKeyword(mToken.mIdent) !=
> >+        eCSSKeyword_at) {
> 
> Just use mToken.mIdent.LowerCaseEqualsLiteral("at") and skip the
> addition of the new token.

Done.

> 
> 
> This is rather an odd way to use eCSSUnit_Function; I think I'd
> prefer if you actually used 2 arguments for a circle and 3 for an
> ellipse.  This would also force you to think about what value
> to use for "unspecified" more clearly, which would avoid the weird
> single-null-value list that you currently produce for the function
> with no arguments.

Ok, I am using new function array items for every value that I add.

> 
> >+  if (!ExpectSymbol(')', true)) {
> >+
> >+    if (mToken.mType != eCSSToken_Ident ||
> >+        nsCSSKeywords::LookupKeyword(mToken.mIdent) !=
> >+        eCSSKeyword_at) {
> 
> I think it matches the expected logic more closely if you have the check
> for the 'at' keyword first, and put the indented section of the function
> (except for the ExpectSymbol(')'), which would move out) inside a test
> for 'at' rather than a test for the failure of ExpectSymbol.
> 
> I also don't know how you even succeed at parsing any of the position
> values, since ExpectSymbol calls UngetToken when it fails, so I don't
> see anything that ever consumes the 'at' token.  Did you actually run
> the tests with the pref enabled?

The problem is the requirement of CSS Shapes. Following is supported for circle(...):

20px at center
at center left 20px
farthest-side
farthest-side at left

The algorithm checks if the first token parses as length/percentage/calc/keyword. However, this can fail like in circle(20at left) or circle(none at left). In this case we don't want to go to the next token but check if the token is an "at" which we do if we know that the token isn't a closing brace. (The empty circle or ellipse is supported as well: circle() or ellipse().) And after that we take the next token for the position.

> 
> nsCSSProps:
> 
> >kEllipseRadiusKTable
> 
> Call this kShapeRadiusKTable to match the spec's term.

Changed it.

> 
> nsCSSValue:
> 
> nsCSSValue::AppendEllipseToString seems like it will mess up
> serializing 'circle()' or 'ellipse()' because of that null value
> I mentioned above.  (Did you run the tests with the pref on?)
> 
> It also seems like the repeated code for the 2 radii could be reduced,
> or put in a function.

The code actually worked in my tests, even though I did indeed accidentally run mochitests with the wrong argument and didn't enable the flag. However, I tested all values manually before anyway and knew that the parsing was correct.

> 
> 
> nsStyleBasicShape should have comments explaining how the radii are stored.

Done.

> 
> 
> I didn't look at the nsComputedDOMStyle code or nsRuleNode code yet,
> but marking review- based on the above.

The nsRuleNode code had to change because of the new parsing code. nsComputedDOMStyle isn't affected by the parser since we have the nsStyleBasicShape representation.

Thanks a lot for your review and suggestions.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, now I see where you're consuming the 'at' token.  The issue is that these two tests:

>+    if (mToken.mType != eCSSToken_Ident ||
>+        !mToken.mIdent.LowerCaseEqualsLiteral("at")) {
>+      REPORT_UNEXPECTED_TOKEN(PEExpectedPosition);
>+      SkipUntil(')');
>+      return false;
>+    }
>+
>+    if (!GetToken(true)) {
>+      REPORT_UNEXPECTED_TOKEN(PEExpectedPosition);
>+      return false;
>+    }

are in the wrong order.  The ExpectSymbol call, however, will have done a GetToken() and then an UngetToken, so the only case where the mToken you're operating on in the first clause will be nonsense is if the ExpectSymbol failed because of EOF (which actually can't happen because ExpectSymbol returns true for ')' at EOF).

But these two if statements should still be in the other order to clarify what you're actually doing.  (In the order you have them, the second will never fail, but it might in the logical order.)


I'll go over the rest of the patch again later.

Comment 10

[Dirk Schulze]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #9)
> Oh, now I see where you're consuming the 'at' token.  The issue is that
> these two tests:
> 
> >+    if (mToken.mType != eCSSToken_Ident ||
> >+        !mToken.mIdent.LowerCaseEqualsLiteral("at")) {
> >+      REPORT_UNEXPECTED_TOKEN(PEExpectedPosition);
> >+      SkipUntil(')');
> >+      return false;
> >+    }
> >+
> >+    if (!GetToken(true)) {
> >+      REPORT_UNEXPECTED_TOKEN(PEExpectedPosition);
> >+      return false;
> >+    }
> 
> are in the wrong order.  The ExpectSymbol call, however, will have done a
> GetToken() and then an UngetToken, so the only case where the mToken you're
> operating on in the first clause will be nonsense is if the ExpectSymbol
> failed because of EOF (which actually can't happen because ExpectSymbol
> returns true for ')' at EOF).
> 
> But these two if statements should still be in the other order to clarify
> what you're actually doing.  (In the order you have them, the second will
> never fail, but it might in the logical order.)

Changing the order does work, yes.

Comment 11

[Dirk Schulze]

Attachment: clip-path-ellipse.patch

I updated the patch.

Fixed an issue where negative radii were possible.
Moved the GetToken check before the "at" keyword check.
Cleaned up nsComputedDOMStyle more.
Added more tests.

Comment 12

[Dirk Schulze]

Attachment: clip-path-ellipse.patch

I updated the patch due to changes in bug 1077388.

Comment 13

[Dirk Schulze]

The grammar for circle() and ellipse() can be found here: http://dev.w3.org/csswg/css-shapes/#funcdef-circle

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8503378 [details] [diff] [review]
clip-path-ellipse.patch




You no longer need the nsCSSKeywordList.h change.  Please remove it.

>+    if (!GetToken(true)) {
>+      REPORT_UNEXPECTED_TOKEN(PEExpectedPosition);
>+      return false;
>+    }

This should use REPORT_UNEXPECTED_EOF instead of REPORT_UNEXPECTED_TOKEN.

(It also needs a different message, e.g.,
PEPositionEOF=<position>
.)


>+    if (mToken.mType != eCSSToken_Ident ||
>+        !mToken.mIdent.LowerCaseEqualsLiteral("at")) {
>+      REPORT_UNEXPECTED_TOKEN(PEExpectedPosition);
>+      SkipUntil(')');
>+      return false;
>+    }
>+
>+    if (!ParsePositionValue(position)) {
>+      REPORT_UNEXPECTED_TOKEN(PEExpectedPosition);
>+      SkipUntil(')');
>+      return false;
>+    }

Please combine these into:

+    if (mToken.mType != eCSSToken_Ident ||
+        !mToken.mIdent.LowerCaseEqualsLiteral("at") ||
+        !ParsePositionValue(position)) {
+      REPORT_UNEXPECTED_TOKEN(PEExpectedPosition);
+      SkipUntil(')');
+      return false;
+    }
+


>+  nsRefPtr<nsCSSValue::Array> functionArray =
>+    aValue.InitFunction(aKeyword, count);

Instead of maintaining the count variable above, set count to 2
for circle and 3 for ellipse.  Then the data are in constant positions,
which is the normal way we handle functions.

nsCSSValue.cpp:

Please rewrite nsCSSValue::AppendCircleOrEllipseToString to handle
the array elements being at constant positions.

(Marking review- because I'd like to look at the revised version.)

nsComputedDOMStyle.cpp:

Please rename BasicShapeTypeToString to AppendBasicShapeTypeToString.

>+    nsROCSSPrimitiveValue* functionNameValue = new nsROCSSPrimitiveValue;

Call this functionValue instead of functionNameValue, since it's got
the whole function in it.

>+        const nsTArray<nsStyleCoord>& radii = aStyleBasicShape->Coordinates();
>+        for (size_t i = 0; i < radii.Length(); ++i) {

Please assert that radii.Length() is 1 for Type::eCircle and 2
for Type::eEllipse.


nsRuleNode.cpp:

Before setting the |basicShape| variable you should assert that it is
null.  (Same for the other setter, existing prior to the patch.)

This code should set the coordinate values to their defaults
(closest-side) even when they're not explicitly specified so that later
code does not need to distinguish between specified-as-default and
unspecified (and thus risk getting one of them wrong).  This should be
slightly more obvious how to do once you've updated this code to deal
with the positions in the array being constant (per-type).

(Also review- because I want to look at this part again.)

nsStyleConsts.h:

>+#define NS_ELLIPSE_RADIUS_CLOSEST  0
>+#define NS_ELLIPSE_RADIUS_FARTHEST 1

Please add "_SIDE" to the end of these.  (There are also closest-corner
and farthest-corner in some contexts.)  You could drop ELLIPSE_ if you
want, though.

nsStyleStruct.h:

>+  // mCoordinates has coordinates for polygon or radii for
>+  // ellipse and circle.

Please move or copy this to above the Coordinates() functions.


property_database.js:

Please add the following invalid_values:
  circle(farthest-side closest-side)
  circle(20% 20%)
  ellipse(farthest-side)
  ellipse(20%)

Comment 15

[Dirk Schulze]

Attachment: clip-path-ellipse-2.patch

I did the requested changes. However, I do not like that we create an array of a fixed number of nsCSSValues at the beginning with possible empty arguments.

If we do not have a radius, the first or the first two nsCSSValue items are of type eCSSUnit_null, or we do not have a position and the last item is of type eCSSUnit_null. (Especially having uneven array length for circle and ellipse makes it tricky to read the code. The same length would be confusing as well though.)

These null types require a lot of extra checking and validation. nsRuleNode but also nsCSSValue have a lot more LOC and are not easier to understand. I tried to make it more readable by using an extra inline function in nsCSSValue which wasn't necessary before. (It also avoids code duplication with the new code.)

I agree with your argument about having NS_RADIUS_CLOSEST_SIDE if there is no specified radius in the nsRuleNode code. But this can be solved by checking the length of Coordinates() at the end of the loop in the old code. If Coordinates() is empty, add NS_RADIUS_CLOSEST_SIDE. (Add it twice for ellipse.)

I added the new tests as well as two other tests and all pass.

Comment 16

[Dirk Schulze]

s/at the end of the loop/after the for loop/

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8504548 [details] [diff] [review]
clip-path-ellipse-2.patch



>+        AppendCircleOrEllipseToString(functionId, aProperty, aResult, aSerialization);

Wrap at less than 80 chars.

>+inline void AppendBasicShapeTypeToString(nsStyleBasicShape::Type aType,
>+                                   nsAutoString& aString)

Fix indent of second line.

nsRuleNode.cpp:

>+        nsStyleCoord radius;
>+        const nsCSSValue& radiusVal = shapeFunction->Item(1);
>+        bool hasRadius = radiusVal.GetUnit() != eCSSUnit_Null;
>+        if (hasRadius) {
>+          DebugOnly<bool> didSetRadius = SetCoord(radiusVal, radius,
>+                                                  nsStyleCoord(), mask,
>+                                                  aStyleContext,
>+                                                  aPresContext,
>+                                                  aCanStoreInRuleTree);
>+          NS_ABORT_IF_FALSE(didSetRadius, "unexpected radius unit");
>+        } else {
>+          radius.SetIntValue(NS_RADIUS_CLOSEST_SIDE, eStyleUnit_Integer);
>+        }
>+        basicShape->Coordinates().AppendElement(radius);
>+
>+        if (type == nsStyleBasicShape::eEllipse) {
>+          nsStyleCoord radiusY;
>+          const nsCSSValue& raduisYVal = shapeFunction->Item(2);
>+          if (hasRadius) {
>+            NS_ABORT_IF_FALSE(raduisYVal.GetUnit() != eCSSUnit_Null,
>+                              "expected vertical radius");
>+            DebugOnly<bool> didSetRadiusY = SetCoord(raduisYVal, radiusY,
>+                                                     nsStyleCoord(), mask,
>+                                                     aStyleContext,
>+                                                     aPresContext,
>+                                                     aCanStoreInRuleTree);
>+            NS_ABORT_IF_FALSE(didSetRadiusY, "unexpected radius unit");
>+          } else {
>+            NS_ABORT_IF_FALSE(raduisYVal.GetUnit() == eCSSUnit_Null,
>+                              "unexpected radius unit");
>+            radiusY.SetIntValue(NS_RADIUS_CLOSEST_SIDE, eStyleUnit_Integer);
>+          }
>+          basicShape->Coordinates().AppendElement(radiusY);
>+        }

This could be a loop from 1 to count.  You can just move the assertion
that the radii units for the ellipse are equal to outside the loop, i.e.:

  NS_ABORT_IF_FALSE(type == nsStyleBasicShape::eCircle ||
                    (shapeFunction->Item(1).GetUnit() == eCSSUnit_Null) ==
                      (shapeFunction->Item(2).GetUnit() == eCSSUnit_Null),
                    "ellipse should have two radii or none");

(Also, s/raduisYVal/radiusYVal/, except you don't need it anymore.)

You can also drop the hasRadius variable and just test
  if (radiusVal.GetUnit() != eCSSUnit_Null)
directly.

>+        if (positionVal.GetUnit() == eCSSUnit_Array) {

Please assert that the value is otherwise null.

r=dbaron with that

(In reply to Dirk Schulze from comment #15)
> I agree with your argument about having NS_RADIUS_CLOSEST_SIDE if there is
> no specified radius in the nsRuleNode code. But this can be solved by
> checking the length of Coordinates() at the end of the loop in the old code.
> If Coordinates() is empty, add NS_RADIUS_CLOSEST_SIDE. (Add it twice for
> ellipse.)

Yeah, that probably would have been ok, although the advantage of doing
things the normal way is that we know it works, and won't run into other
problems that we didn't think of.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and I think this:

>+          radius.SetIntValue(NS_RADIUS_CLOSEST_SIDE, eStyleUnit_Integer);

should use eStyleUnit_Enumerated instead of eStyleUnit_Integer.  It should definitely match what SetCoord produces for the keyword in question.

(I suspect you were serializing those values incorrectly!  Would be nice to have a test that catches that.)

Comment 19

[Dirk Schulze]

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #17)
> Comment on attachment 8504548 [details] [diff] [review]
> clip-path-ellipse-2.patch
> 
> 
> 
> >+        AppendCircleOrEllipseToString(functionId, aProperty, aResult, aSerialization);
> 
> Wrap at less than 80 chars.

Fixed.

> 
> >+inline void AppendBasicShapeTypeToString(nsStyleBasicShape::Type aType,
> >+                                   nsAutoString& aString)
> 
> Fix indent of second line.

Fixed.

> 
> nsRuleNode.cpp:
> 
> >+        nsStyleCoord radius;
> 
> This could be a loop from 1 to count.  You can just move the assertion
> that the radii units for the ellipse are equal to outside the loop, i.e.:
> 
>   NS_ABORT_IF_FALSE(type == nsStyleBasicShape::eCircle ||
>                     (shapeFunction->Item(1).GetUnit() == eCSSUnit_Null) ==
>                       (shapeFunction->Item(2).GetUnit() == eCSSUnit_Null),
>                     "ellipse should have two radii or none");

Done.

> 
> (Also, s/raduisYVal/radiusYVal/, except you don't need it anymore.)
> 
> You can also drop the hasRadius variable and just test
>   if (radiusVal.GetUnit() != eCSSUnit_Null)
> directly.

Removed the boolean.

> 
> >+        if (positionVal.GetUnit() == eCSSUnit_Array) {
> 
> Please assert that the value is otherwise null.

Added the assert.

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #18)
> Oh, and I think this:
> 
> >+          radius.SetIntValue(NS_RADIUS_CLOSEST_SIDE, eStyleUnit_Integer);
> 
> should use eStyleUnit_Enumerated instead of eStyleUnit_Integer.  It should
> definitely match what SetCoord produces for the keyword in question.
> 
> (I suspect you were serializing those values incorrectly!  Would be nice to
> have a test that catches that.)

Fixed. I investigated why it was not failing with the existing tests. The problem is that NS_RADIUS_CLOSEST_SIDE is 0 and circle(at center) would get to circle(0 at 50% 50%) for computed style which is still a valid syntax according the CSS Shapes. Even though it does something different. I switched the values of NS_RADIUS_CLOSEST_SIDE and NS_RADIUS_FARTHEST_SIDE since I expect the default to trigger more often. The computed style would get to circle(1 at 50% 50%) which is invalid. The tests cover this problem now.

Comment 20

[Dirk Schulze]

Attachment: clip-path-ellipse.patch

Patch for landing. Try server results: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=58c0541fcb5e

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68308752b11b

Comment 22

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/68308752b11b