HTMLTableColElement span should be unsigned long

RESOLVED DUPLICATE of bug 824907

Status

()

defect
RESOLVED DUPLICATE of bug 824907
8 years ago
7 years ago

People

(Reporter: dzbarsky, Assigned: atulagrwl)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Blocks: 586126
I assume that as you file these you're checking what browsers actually implement right now (that is, whether the spec is sane)?
Assignee: nobody → atulagrwl
Posted patch Patch v1 (obsolete) — Splinter Review
Attachment #558068 - Flags: review?(Olli.Pettay)
Status: NEW → ASSIGNED
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.
Posted patch Patch v2Splinter Review
Accommodating changes suggested by Ms2ger.
Attachment #558068 - Attachment is obsolete: true
Attachment #558068 - Flags: review?(Olli.Pettay)
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.
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.
Do we really want this limitation to be part of the attribute reflection? it could be part of the layout code too.
I'm fine either way; what do other UAs do?
(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.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 824907
You need to log in before you can comment on or make changes to this bug.