Bug 173049 – Align <col span>/<colgroup span> limits with the latest HTML specification

• Component: DOM
• Status: RESOLVED FIXED

[Simon Pieters (:zcorpan)]

This is the same as https://bugs.webkit.org/show_bug.cgi?id=171322 but for the IDL attributes for <col span> and <colgroup span>.

https://github.com/whatwg/html/issues/2705
https://github.com/whatwg/html/pull/2734
https://github.com/w3c/web-platform-tests/pull/6172

/html/semantics/tabular-data/processing-model-1/col-span-limits.html fails in WebKit with

Fail	col span of 1001 must be treated as 1000	assert_equals: table3 width expected 51 but got 53

/html/dom/reflection-tabular.html fails in WebKit with, among others:

Fail	colgroup.span: setAttribute() to 2147483647	assert_equals: IDL get expected 1000 but got 2147483647

[Chris Dumez]

Created attachment 312221 [details]
WIP Patch

[Chris Dumez]

Created attachment 312227 [details]
Patch

[Daniel Bates]

Comment on attachment 312227 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312227&action=review

> Source/WebCore/ChangeLog:8
> +        Align<col span>/<colgroup span> limits with the latest HTML specification after:

Missing a space after "Align".

> Source/WebCore/html/HTMLTableColElement.cpp:39
> +static const unsigned defaultSpan { 1 };
> +static const unsigned minSpan { 1 };
> +static const unsigned maxSpan { 1000 };

The keyword static is unnecessary. In C++, const has internal linkage in file scope.

Do we use C++ brace initialization for POD constants?

> Source/WebCore/html/HTMLTableColElement.cpp:99
> +    setUnsignedIntegralAttribute(spanAttr, limitToOnlyHTMLNonNegative(span, defaultSpan));

Please add a remark to the ChangeLog about this change.

It seems weird that setting the span programmatically has different behavior than parsing the HTML span attribute.

[Chris Dumez]

Comment on attachment 312227 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312227&action=review

>> Source/WebCore/html/HTMLTableColElement.cpp:39
>> +static const unsigned maxSpan { 1000 };
> 
> The keyword static is unnecessary. In C++, const has internal linkage in file scope.
> 
> Do we use C++ brace initialization for POD constants?

We are using curly brackets for more and more things AFAIK.

>> Source/WebCore/html/HTMLTableColElement.cpp:99
>> +    setUnsignedIntegralAttribute(spanAttr, limitToOnlyHTMLNonNegative(span, defaultSpan));
> 
> Please add a remark to the ChangeLog about this change.
> 
> It seems weird that setting the span programmatically has different behavior than parsing the HTML span attribute.

It doesn't really. Calling setUnsignedIntegralAttribute() will end up calling HTMLTableColElement::parseAttribute() for the span attribute, so the logic above will apply.

[Chris Dumez]

Created attachment 312241 [details]
Patch

[Chris Dumez]

Comment on attachment 312241 [details]
Patch

Clearing flags on attachment: 312241

Committed r217907: <http://trac.webkit.org/changeset/217907>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.