813034 - Implement table.createTBody

Status: RESOLVED FIXED

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

Description

[Kohei Yoshino]

Spec: http://dev.w3.org/html5/spec/the-table-element.html#dom-table-createtbody
WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=84465

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Whoever implements this, please use WhatWG spec
http://www.whatwg.org/specs/web-apps/current-work/multipage/tabular-data.html#dom-table-createtbody

Comment 2

[Josh Matthews [:jdm]]

Relevant:
* http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/html/nsIDOMHTMLTableElement.idl
* http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTableElement.cpp

Comment 3

[Marc Jessome[:mjessome]]

Attachment: table.createTBody v1

Try results:
https://tbpl.mozilla.org/?tree=Try&rev=2a89c6831c65

Comment 4

[Marc Jessome[:mjessome]]

I've just realized that the deleteTBody(), which I had implemented to delete the first TBody element, is not a part of the spec. Does this also remove the need for GetTBody() as implemented?

Comment 5

[Marc Jessome[:mjessome]]

Attachment: Testcase1

Comment 6

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

Could you upload a patch to implement just createTBody, as the others don't exist in the spec?

Comment 7

[Marc Jessome[:mjessome]]

Attachment: table.createTBody v2

Remove implementation of deleteTBody() as it is not in spec and remove implementation of getTBody().

Comment 8

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

Comment on attachment 688795 [details] [diff] [review]
table.createTBody v2

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

Generally looks good; will need tests, though.

::: content/html/content/src/nsHTMLTableElement.cpp
@@ +593,5 @@
> +    *aValue = nullptr;
> +
> +    nsCOMPtr<nsINodeInfo> nodeInfo;
> +    nsContentUtils::NameChanged(mNodeInfo, nsGkAtoms::tbody,
> +                                getter_AddRefs(nodeInfo));

Need to check for out-of-memory here

@@ +601,5 @@
> +    if (!newBody) {
> +        return NS_OK;
> +    }
> +
> +    AppendChildTo(newBody, true);

This is incorrect; you need to add the new tbody immediately after the last tbody element in the table. For example, calling createTBody on the following table:

<table>
<tbody></tbody>
<tfoot></tfoot>
</table>

should insert the new tbody between the tbody and the tfoot.

Comment 9

[Marc Jessome[:mjessome]]

Attachment: Testcase v2

Comment 10

[Marc Jessome[:mjessome]]

Attachment: table.createTBody v3

This will add the new TBody element after the last one if there is one that exists, or as the last element of the table as the spec states.

I am unsure of what should be done in the case that "lastBody" cannot be retrieved.

Comment 11

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

Comment on attachment 689242 [details] [diff] [review]
table.createTBody v3

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

This can be made somewhat more efficient by using nsINode APIs:

::: content/html/content/src/nsHTMLTableElement.cpp
@@ +604,5 @@
> +        return NS_OK;
> +    }
> +
> +    nsCOMPtr<nsIDOMHTMLCollection> bodies;
> +    GetTBodies(getter_AddRefs(bodies));

nsContentList* bodies = TBodies();

@@ +607,5 @@
> +    nsCOMPtr<nsIDOMHTMLCollection> bodies;
> +    GetTBodies(getter_AddRefs(bodies));
> +
> +    uint32_t bodyCount;
> +    bodies->GetLength(&bodyCount);

uint32_t bodyCount = bodies->GetLength();

@@ +609,5 @@
> +
> +    uint32_t bodyCount;
> +    bodies->GetLength(&bodyCount);
> +
> +    nsCOMPtr<nsIDOMNode> nextElem;

nsIContent* nextElem = nullptr;

@@ +615,5 @@
> +      // Already have tbody elements, insert after the last one
> +      nsCOMPtr<nsIDOMNode> lastBody;
> +      bodies->Item(bodyCount - 1, getter_AddRefs(lastBody));
> +
> +      lastBody->GetNextSibling(getter_AddRefs(nextElem));

Element* lastBody = bodies->GetElementAt(bodyCount - 1);
nextElem = lastBody->GetNextSibling();

@@ +628,5 @@
> +      nsresult r;
> +      // First tbody element
> +      r = AppendChildTo(newBody, true);
> +      NS_ENSURE_SUCCESS(r, r);
> +    }

This can all be replaced by

ErrorResult rv;
InsertBefore(*newBody, nextElem, rv);
if (rv.Failed()) {
  return rv.ErrorCode();
}

Comment 12

[Marc Jessome[:mjessome]]

Attachment: table.createTBody v4

Comment 13

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

Hey Marc,

Sorry I let this patch fall through the cracks. It'll need a bit of updating to the new WebIDL bindings. Let me know if you want to do that; else I'll look at it next week or so.

Comment 14

[Marc Jessome[:mjessome]]

I'd be curious to see some information on using the WebIDL bindings, as I am unfamiliar with them. I'd be interested in looking at it, but it would obviously take me a bit of time to look at it.

Comment 15

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

There is some documentation at <https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings>; it would be useful if you let us know if anything you need to get started isn't documented there.

Comment 16

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

Stealing this; looking at writing tests now.

Comment 17

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

Attachment: Patch v5

I updated this.

Comment 18

[Mounir Lamouri (:mounir)]

Comment on attachment 769329 [details] [diff] [review]
Patch v5

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

r=me but I would like Olli to do the spec review. I do not know much about table.

Also, why are you removing the test instead of moving it out of the failure directory?

Comment 19

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

All I'm removing is the list of expected failures, not the test.

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/33290309ae97