668819 - HTMLTableColElement span should be unsigned long

Status: RESOLVED DUPLICATE of bug 824907

(Core :: DOM: Core & HTML, defect)

Description

[David Zbarsky (:dzbarsky)]

See http://www.whatwg.org/specs/web-apps/current-work/multipage/tabular-data.html#htmltablecolelement

Comment 1

[Boris Zbarsky [:bzbarsky]]

I assume that as you file these you're checking what browsers actually implement right now (that is, whether the spec is sane)?

Comment 2

[Atul Aggarwal]

Attachment: Patch v1

Comment 3

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

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

>--- /dev/null
>+++ b/content/html/content/test/test_bug668819.html

Please rename the test "test_colgroup_attributes_reflection.html" and add a "test_col_attributes_reflection.html" that does the same for "col" elements.

>+reflectUnsignedInt({
>+  element: colgroup, 
>+  attribute: "span", 
>+  nonZero: true, 
>+  defaultValue: 1,
>+});

And drop the defaultValue, it's not necessary.

Comment 4

[Atul Aggarwal]

Attachment: Patch v2

Accommodating changes suggested by Ms2ger.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 558070 [details] [diff] [review]
Patch v2

Review of attachment 558070 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLTableColElement.cpp
@@ +113,5 @@
>  
>  NS_IMPL_STRING_ATTR(nsHTMLTableColElement, Align, align)
>  NS_IMPL_STRING_ATTR(nsHTMLTableColElement, Ch, _char)
>  NS_IMPL_STRING_ATTR(nsHTMLTableColElement, ChOff, charoff)
> +NS_IMPL_UINT_ATTR_NON_ZERO_DEFAULT_VALUE(nsHTMLTableColElement, Span, span, 1)

Could you update the ::ParseAttribute() method? There is a max value for span and I wonder why and if we can remove it. This is unlikely following the specs.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The max value is there to prevent accidental DoS via huge colspan values, because on the layout side CSS requires storing information in tables on a per-logical-cell basis, so the amount of information stored is O(N^M) where N is number of logical rows and M is number of columns.  If you have <td rowspan="1000000" colspan="1000000"> then layout needs to store 1e12 pieces of data.  Needless to say, this does not work well.

Furthermore, last I checked there was some cross-browser compat on the exact caps for rowspan and colspan, or at least we and WebKit and maybe Presto had coordinated ours.  So if that's not what the spec says, the spec probably needs changing.

Comment 7

[Mounir Lamouri (:mounir)]

Do we really want this limitation to be part of the attribute reflection? it could be part of the layout code too.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'm fine either way; what do other UAs do?

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> I'm fine either way; what do other UAs do?

Webkit and Gecko seems to have the same behavior that isn't the one the spec requires.
Presto and IE9 reflects the value as expected (with a few bugs not related to this specific attribute).

It would be interesting to know what IE6 does. The specs might request that for retro-compatibility.