Bug 159583 – Setting table.tFoot or calling table.createTFoot() should append HTML tfont element to the end of the table

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Daniel Bates]

As per the HTML standard, assignment to table.tFoot and invoking table.createTFoot() should append an HTML tfoot element at the end of the table:

[[
The tFoot IDL attribute must return, on getting, the first tfoot element child of the table element, if any, or null otherwise. On setting, if the new value is null or a tfoot element, the first tfoot element child of the table element, if any, must be removed, and the new value, if not null, must be inserted at the end of the table. If the new value is neither null nor a tfoot element, then a "HierarchyRequestError" DOMException must be thrown instead.

The createTFoot() method must return the first tfoot element child of the table element, if any; otherwise a new tfoot element must be table-created and inserted at the end of the table, and then that new element must be returned.
]]
<https://html.spec.whatwg.org/multipage/tables.html#dom-table-tfoot> (8 July 2016)

Currently the behavior of table.tFoot and table.createTFoot() conform to earlier revisions of the HTML5 spec. In such revisions assignment to table.tFoot or calling table.createTFoot() would insert the HTML tfoot element "immediately before the first element in the table element that is neither a caption element, a colgroup element, nor a thead element, if any, or at the end of the table if there are no such elements" (https://www.w3.org/TR/2008/WD-html5-20080122).

[Radar WebKit Bug Importer]

<rdar://problem/27255292>

[Daniel Bates]

Created attachment 283204 [details]
Patch

[Chris Dumez]

Comment on attachment 283204 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        (WebCore::HTMLTableElement::setTFoot): Append <tfoot> to the end of the table.

The issue is that right now, I believe we match Chrome's behavior. Only Firefox matches the spec. I haven't tested Edge.

[Chris Dumez]

Comment on attachment 283204 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        (WebCore::HTMLTableElement::setTFoot): Append <tfoot> to the end of the table.
> 
> The issue is that right now, I believe we match Chrome's behavior. Only Firefox matches the spec. I haven't tested Edge.

That said, I much prefer the spec's behavior. I just think we should confirm Edge matches the spec.

> Source/WebCore/html/HTMLTableElement.cpp:137
> +    appendChild(*newFoot, ec);

Since we're touching this function, it would be nice to use RefPtr<>&& instead of PassRefPtr<>.

[Daniel Bates]

(In reply to comment #3)
> Comment on attachment 283204 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283204&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        (WebCore::HTMLTableElement::setTFoot): Append <tfoot> to the end of the table.
> 
> The issue is that right now, I believe we match Chrome's behavior. Only
> Firefox matches the spec. I haven't tested Edge.

Edge and Internet Explorer version 8 (*) and later also match the spec. and the behavior proposed in this patch.

(*) I used Internet Explorer 11 (version 11.0.9600.18321) on Windows 8 to test the behavior in IE 11 and varied its compatibility mode setting.

[Chris Dumez]

Comment on attachment 283204 [details]
Patch

Ok, thanks for checking. r=me

[Daniel Bates]

(In reply to comment #4)
> Since we're touching this function, it would be nice to use RefPtr<>&&
> instead of PassRefPtr<>.

Will fix before landing.

[Build Bot]

Comment on attachment 283204 [details]
Patch

Attachment 283204 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1649152

New failing tests:
tables/mozilla/bugs/bug30418.html

[Build Bot]

Created attachment 283219 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Daniel Bates]

Committed r203009: <http://trac.webkit.org/changeset/203009>

[Daniel Bates]

(In reply to comment #10)
> Committed r203009: <http://trac.webkit.org/changeset/203009>

I inadvertently used WTFMove() in HTMLTableElement::createTFoot() to move the HTML tfoot element into the argument passed to HTMLTableElement::setTFoot(). We should use Ref::copyRef() instead. Committed fix in <https://trac.webkit.org/changeset/203011>.

[Darin Adler]

(In reply to comment #11)
> I inadvertently used WTFMove() in HTMLTableElement::createTFoot() to move
> the HTML tfoot element into the argument passed to
> HTMLTableElement::setTFoot(). We should use Ref::copyRef() instead.
> Committed fix in <https://trac.webkit.org/changeset/203011>.

And we should have a test case covering this behavior so we can’t make the mistake again in the future.

[Chris Dumez]

(In reply to comment #12)
> (In reply to comment #11)
> > I inadvertently used WTFMove() in HTMLTableElement::createTFoot() to move
> > the HTML tfoot element into the argument passed to
> > HTMLTableElement::setTFoot(). We should use Ref::copyRef() instead.
> > Committed fix in <https://trac.webkit.org/changeset/203011>.
> 
> And we should have a test case covering this behavior so we can’t make the
> mistake again in the future.

I would expect the following test to cover this already:
LayoutTests/imported/w3c/web-platform-tests/html/semantics/tabular-data/the-table-element/tFoot.html

The issue is that the patch that was reviewed and went through EWS did not have the bug. The bug was introduced before landing when fixing a nit.