102344 - 'border-spacing' property in CSS2 cannot have negative lengths

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tete]

In the following HTML table, the 'table' element has two CSS2 declarations.
In CSS2, the 'border-spacing' property cannot have negative lengths.
Accordingly, a CSS2 user agent must ignore the declaration.

But, Mozilla 0.9.4 accepts it.


<table border="1" style="border-collapse: separate; border-spacing: -0.5em">
<tr>
<td>ABCDEFG</td>
<td>HIJKLMN</td>
</tr>
<tr>
<td>OPQRSTU</td>
<td>VWXYZ</td>
</tr>
</table>


References:
http://www.w3.org/TR/REC-CSS2/tables.html#propdef-border-spacing
http://www.w3.org/TR/html401/struct/tables.html

Comment 1

[Boris Zbarsky [:bzbarsky]]

Indeed.  Over to style system....

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yep.  Confirming.  Either we need a new variant for non-negative lengths or 
ParseBorderSpacing needs to do the checking itself.

Comment 3

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

We should probably do whatever we do for padding, which also cannot accept
negative lengths.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Looks like that would just be a matter of replacing ParseVariant with
ParsePositiveVariant in CSSParserImpl::ParseBorderSpacing

Attaching patch

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix this

Comment 6

[Boris Zbarsky [:bzbarsky]]

reviews?

Comment 7

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

The way you use ExpectEndProperty will mess up error reporting.  Perhaps it
would be better to call ExpectEndProperty in the else of the second
ParsePositiveVariant check and move the AppendValue calls for the one-value case
there as well?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Well, it looks like ParsePositiveVariant() will eat up tokens... so something
like:

border-spacing: 0.5em -2em;

would succeed on the first ParsePositiveVariant, fail on the second.  Now what
do we do?  Just use 0.5em for both dimensions?  We have no way of telling
whether the second ParsePositiveVariant failed because there was nothing there
to parse or because it parsed an illegal value.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Would it make sense for ParsePositiveVariant to UngetToken() for cases when
ParseVariant() succeeds but returns a negative value?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Better patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 51422 [details] [diff] [review]
Patch to fix this

The second patch fixes ParsePositiveVariant() so we handle this and negative paddings better and uses it...

Comment 12

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

Comment on attachment 51483 [details] [diff] [review]
Better patch

r=dbaron

Comment 13

[Marc Attinasi]

Comment on attachment 51483 [details] [diff] [review]
Better patch

sr=attinasi

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked into trunk.