414638 - Disallow mixed space/comma syntax for CSS rect() value

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://lists.w3.org/Archives/Public/www-style/2008Jan/0208.html

We allow as per spec
    clip: rect(96px, 96px, 96px, 96px);
and for compatibility we allow
    clip: rect(96px 96px 96px 96px);
but we also allow
    clip: rect(96px  ,96px 96px,  96px);
and we probably shouldn't.

Comment 2

[bluepheasant]

Attachment: testcase

Comment 3

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

The spec has finally clarified this to say that all-commas and no-commas are allowed, but mixed is not, so we should in fact change.

http://test.csswg.org/suites/css2.1/20101001/html4/clip-rect-001.htm

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

I'm assuming that fantasai doesn't mind me reusing her test...

Comment 5

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

Comment on attachment 528065 [details] [diff] [review]
Patch v1

>From: Ms2ger <[e-mail]>
>Bug 414638 - Disallow mixed space/comma syntax for CSS rect() value

You may want to get a newer version of Mercurial at some point.

>+    PRBool useCommas = PR_FALSE;

Better to leave this uninitialized so valgrind will catch any errors.

>     NS_FOR_CSS_SIDES(side) {
>       if (! ParseVariant(rect.*(nsCSSRect::sides[side]),
>                          VARIANT_AL, nsnull)) {
>         return PR_FALSE;
>       }
>-      if (side < 3) {
>-        // skip optional commas between elements
>-        (void)ExpectSymbol(',', PR_TRUE);
>+      if (side == 0) {
>+        useCommas = ExpectSymbol(',', PR_TRUE);
>+      } else if (side < 3) {
>+        // Skip optional commas between elements, but only if the first
>+        // separator was a comma.
>+        if (ExpectSymbol(',', PR_TRUE) != useCommas) {
>+          return PR_FALSE;
>+        }

You can make this addition simpler (and avoid the highly unusual pattern of comparing the result of ExpectSymbol to some other boolean) by changing it to:

if (side == 0) {
  useCommas = ExpectSymbol(',', PR_TRUE);
} else if (useCommas && side < 3) {
  if (!ExpectSymbol(',', PR_TRUE)) {
    return PR_FALSE;
  }
}

r=dbaron with that.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Will fix. Thanks.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/2aa25183ddec