The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla6

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: roc, Assigned: Ms2ger)

Tracking

(Blocks: 1 bug)

Trunk
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.

Updated

9 years ago
Duplicate of this bug: 457655

Comment 2

9 years ago
Created attachment 340904 [details]
testcase
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
Blocks: 605520
Whiteboard: [good first bug]
(Assignee)

Updated

6 years ago
Assignee: nobody → Ms2ger
Whiteboard: [good first bug] → [needs patch]
(Assignee)

Comment 4

6 years ago
Created attachment 528065 [details] [diff] [review]
Patch v1

I'm assuming that fantasai doesn't mind me reusing her test...
Attachment #528065 - Flags: review?(dbaron)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Whiteboard: [needs patch] → [needs review]
Target Milestone: --- → mozilla6
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.
Attachment #528065 - Flags: review?(dbaron) → review+
(Assignee)

Comment 6

6 years ago
Will fix. Thanks.
Whiteboard: [needs review] → [needs landing]
(Assignee)

Comment 7

6 years ago
http://hg.mozilla.org/mozilla-central/rev/2aa25183ddec
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.