Last Comment Bug 414638 - Disallow mixed space/comma syntax for CSS rect() value
: Disallow mixed space/comma syntax for CSS rect() value
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
: Jet Villegas (:jet)
Mentors:
: 457655 (view as bug list)
Depends on:
Blocks: css2.1-tests
  Show dependency treegraph
 
Reported: 2008-01-29 11:21 PST by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2011-05-08 13:55 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (427 bytes, text/html)
2008-09-29 04:30 PDT, bluepheasant
no flags Details
Patch v1 (4.60 KB, patch)
2011-04-25 00:51 PDT, :Ms2ger (⌚ UTC+1/+2)
dbaron: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2008-01-29 11:21:48 PST
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 1 bluepheasant 2008-09-29 04:29:29 PDT
*** Bug 457655 has been marked as a duplicate of this bug. ***
Comment 2 bluepheasant 2008-09-29 04:30:23 PDT
Created attachment 340904 [details]
testcase
Comment 3 David Baron :dbaron: ⌚️UTC-10 2010-10-14 13:31:06 PDT
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 (⌚ UTC+1/+2) 2011-04-25 00:51:51 PDT
Created attachment 528065 [details] [diff] [review]
Patch v1

I'm assuming that fantasai doesn't mind me reusing her test...
Comment 5 David Baron :dbaron: ⌚️UTC-10 2011-05-02 14:19:15 PDT
Comment on attachment 528065 [details] [diff] [review]
Patch v1

>From: Ms2ger <ms2ger@gmail.com>
>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 (⌚ UTC+1/+2) 2011-05-03 14:49:24 PDT
Will fix. Thanks.
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2011-05-08 12:38:22 PDT
http://hg.mozilla.org/mozilla-central/rev/2aa25183ddec

Note You need to log in before you can comment on or make changes to this bug.