685400 - Add a new syntax to -moz-linear-gradient per latest css spec

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Masatoshi Kimura [:emk]]

The meaning of <angle> has been changed. In the past, it was:
http://www.w3.org/TR/2011/WD-css3-images-20110217/#linear-gradient-syntax
> The first is by specifying the angle the gradient-line should assume; this uses the standard algebraic notation for angles where 0deg points to the right, 90deg points up, and positive angles go counterclockwise.

Now it has been changed to the following:
http://www.w3.org/TR/2011/WD-css3-images-20110712/#linear-gradient-syntax
> The first is by specifying the angle the gradient-line should assume; for the purposes of this argument, 0deg points upwards, 90deg points toward the right, and positive angles go clockwise.

At present Mozilla implements the former.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Part1: add "to" syntax

This patch also enables "magic corner" feature.

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Part2: change a meaning of angle

Comment 3

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

Given that the spec is likely to reach CR soon, I think it may be better if we don't change the -moz- syntax (which many authors are using) until we unprefix it.

Comment 4

[Masatoshi Kimura [:emk]]

Can we unprefix -moz-linear-gradient as soon as the spec reach CR stage?
If we can't, I propose appling only Part 1 so that authors can test new "magic corner" feature. Part 1 is 100% backward compatible with the current syntax.

BTW, it sounds odd to me the spec can become CR without any implemantation (no browsers implement a new syntax).

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Part1: add "to" syntax

Use atan2 instead of cryptic formulas

Comment 6

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

I started looking at the spec to see what it is you were implementing (in order to review the patch), and then sent:
http://lists.w3.org/Archives/Public/www-style/2011Sep/0174.html .  The patch may need updating depending on the reply.

Comment 7

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

Comment on attachment 559772 [details] [diff] [review]
Part1: add "to" syntax

Also, looking through the code, I notice some comments in nsCSSParser.cpp, in particular these:

>-// <gradient-line> : [<bg-position> || <angle>] ,
>+// <gradient-line> : [ 'to'? <bg-position> || <angle>] ,

>+      // "to" syntax only allows an angle alone

which seem incorrect (compared to the spec).  Are the comments wrong and the code correct, or is there code that needs to be fixed here?

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to David Baron [:dbaron] from comment #6)
In my understanding that start, end, before, or after were postponed to CSS4.
See list messages starting from
<http://lists.w3.org/Archives/Public/www-style/2011Aug/0281.html>.
I think someone had forgotten to remove logical keywords from the repeating-
gradients section.

(In reply to David Baron [:dbaron] from comment #7)
> >-// <gradient-line> : [<bg-position> || <angle>] ,
> >+// <gradient-line> : [ 'to'? <bg-position> || <angle>] ,
The comment should be:
<gradient-line> : [ to [left | right] || [top | bottom] ] ,
                | [ <bg-position> || <angle>] , # for compatibility with legacy spec.
I'll update the comment in the next patch.

> >+      // "to" syntax only allows an angle alone
I'm trying to say that something like "to left top 90deg" are not allowed 
(contrary to the legacy spec which allows "left top 90deg").
Only "to left top" or "90deg" are valid.
Note that new <angle> syntax is not included in this patch 
because it is incompatible with the old syntax.

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: Part1: add "to" syntax and "magic corner" feature

comments updated

Comment 10

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

Comment on attachment 559795 [details] [diff] [review]
Part1: add "to" syntax and "magic corner" feature

In ParseGradient, near the start, you add:

>+  PRBool toCorner = PR_FALSE;
>+  if (id.LowerCaseEqualsLiteral("to")) {

You need to check that |ty| is eCSSToken_Ident before checking the value
of |id|; some token types don't fill in ident and leave it whatever it
was before (though that technically wouldn't matter in this case) and it
could be one of the other token types that sets mIdent (which does
matter in this case -- it looks like your code accepts "5to" or "to(" in
addition to just "to").

I think you're also better off adding this check before setting |ty|
and |id| the first time and making it just:

PRBool toCorner = PR_FALSE;
if (mToken.mType == eCSSToken_Ident &&
    mToken.mIdent.LowerCaseEqualsLiteral("to")) {
  toCorner = PR_TRUE;
  if (!GetToken(PR_TRUE)) {
    return PR_FALSE;
  }
}

(You'll also need to adjust the patch for the PRBool changes; there are
scripts for that; see:
https://blog.mozilla.com/mwu/2011/09/29/mozilla-central-now-with-88-7-less-prbool/
https://bugzilla.mozilla.org/show_bug.cgi?id=690892
... but whether you need the second depends on when you update it.)

>+      // "to" syntax cannot appear along with <angle>
>+      if (toCorner && haveAngle) {
>+        SkipUntil(')');
>+        return PR_FALSE;
>+      }

This code needs to move above the "if" that it's in.  It looks like
your code incorrectly accepts "to <angle>" as a result.  You should
add a test and make sure the test fails without the change.

>+      // "to" syntax only allows box position keywords
>+      if (toCorner) {
>+        const nsCSSValue& xValue = cssGradient->mBgPos.mXValue;
>+        const nsCSSValue& yValue = cssGradient->mBgPos.mYValue;
>+        if (xValue.GetUnit() != eCSSUnit_Enumerated ||
>+            !(xValue.GetIntValue() & (NS_STYLE_BG_POSITION_LEFT |
>+                                      NS_STYLE_BG_POSITION_CENTER |
>+                                      NS_STYLE_BG_POSITION_RIGHT)) ||
>+            yValue.GetUnit() != eCSSUnit_Enumerated ||
>+            !(yValue.GetIntValue() & (NS_STYLE_BG_POSITION_TOP |
>+                                      NS_STYLE_BG_POSITION_CENTER |
>+                                      NS_STYLE_BG_POSITION_BOTTOM))) {
>+          SkipUntil(')');
>+          return PR_FALSE;
>+        }
>+      }

These tests should not allow center (for either x or y).


That said, I think it would be much clearer and also simpler to rewrite
these parsing changes so that instead of trying to parse the 'to' case
with the same parsing code that parses the other, that it instead just
have its own branch inside the if (haveGradientLine) code.  Then you
don't need the extra token thing (you can just make a "to" token set
haveGradientLine to true).  Instead, just have an additional branch for
the "to" syntax and give it its own call to ParseBoxPositionValues.
Please do that instead of addressing the above comments one-by-one.


In test_unclosed_parentheses.html you should also add tests for:
  -moz-linear-gradient(to
  -moz-linear-gradient(to top
  -moz-linear-gradient(to top left
  -moz-linear-gradient(to top left,


I'd like to review another version of this patch because I'd like
to review the changes to the parsing code, but otherwise this looks
great.

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: add "to" syntax and "magic corner" feature

(In reply to David Baron [:dbaron] from comment #10)
> This code needs to move above the "if" that it's in.  It looks like
> your code incorrectly accepts "to <angle>" as a result.  You should
> add a test and make sure the test fails without the change.
The patch already contains
>			"-moz-linear-gradient(to -33deg, red, blue)",
and
>			"-moz-repeating-linear-gradient(to -33deg, red, blue)",
as a test for invalid values. Anyway, I've completely rewritten this code as you suggested in the later comment.

> >+      // "to" syntax only allows box position keywords
> >+      if (toCorner) {
> >+        const nsCSSValue& xValue = cssGradient->mBgPos.mXValue;
> >+        const nsCSSValue& yValue = cssGradient->mBgPos.mYValue;
> >+        if (xValue.GetUnit() != eCSSUnit_Enumerated ||
> >+            !(xValue.GetIntValue() & (NS_STYLE_BG_POSITION_LEFT |
> >+                                      NS_STYLE_BG_POSITION_CENTER |
> >+                                      NS_STYLE_BG_POSITION_RIGHT)) ||
> >+            yValue.GetUnit() != eCSSUnit_Enumerated ||
> >+            !(yValue.GetIntValue() & (NS_STYLE_BG_POSITION_TOP |
> >+                                      NS_STYLE_BG_POSITION_CENTER |
> >+                                      NS_STYLE_BG_POSITION_BOTTOM))) {
> >+          SkipUntil(')');
> >+          return PR_FALSE;
> >+        }
> >+      }
> 
> These tests should not allow center (for either x or y).
ParseBoxPositionValues will set NS_STYLE_BG_POSITION_CENTER even if "center" keyword does not appear explicitly. For example, it will treat "top" as "top center". I've added an argument to ParseBoxPositionValues to handle this case.

Comment 12

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

Comment on attachment 564485 [details] [diff] [review]
add "to" syntax and "magic corner" feature

>Bug 685400 - add "to" syntax and "magic corner" feature

This should at least say "to CSS gradient syntax" at the end.

Change the "aDenyExplicitCenter" to "aAllowExplicitCenter" and invert 
the meaning; parameters named in terms of negatives are confusing.

>+      aDenyExplicitCenter && mask & BG_CENTER) {

two pairs of parentheses would make this clearer (one for the &&
and one for the &)


r=dbaron with that.

Sorry for the delay.

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: patch for check in

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: patch for check in

The previous patch has been bitrotted by bug 659963. Unbitrotted.

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5cfb2cfe8beb

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5cfb2cfe8beb

Comment 17

[Jean-Yves Perrier [:teoli]]

I am in the process of documenting this for the MDN. Am I correct when I understand thus
1) the old syntax -moz-linear-gradient(top left ...) is still accepted, for compatibility purpose. It will be removed when the -moz will be removed. (It is the same as -moz-linear-gradient(to bottom right ...) )
2) the 0deg <angle> is still horizontal and will be changed to vertical when the -moz will be removed.
3) There is currently no bug opened for these two changes (I wanted to add dev-doc-needed to it :-) ), nor for the (-moz-)radial-gradient "at" syntax.

?

Thank you in advance.

Comment 18

[Masatoshi Kimura [:emk]]

(In reply to Jean-Yves Perrier [:teoli] from comment #17)
1) right.
2) the 0deg would be vertical when using unprefixed syntax and would be horizontal when using prefixed syntax for compatibility.
3) The first half is right. The second half is bug 627885.

Comment 19

[Jean-Yves Perrier [:teoli]]

I've updated the docs:
https://developer.mozilla.org/en/CSS/linear-gradient
https://developer.mozilla.org/en/CSS/repeating-linear-gradient
https://developer.mozilla.org/en/CSS/Using_CSS_gradients

(and created an article for the <image> and <gradient> CSS types at the same time).

I also upgraded Fx 10 for developers, of course.