Implement table.createTBody

RESOLVED FIXED in mozilla25

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kohei, Assigned: Ms2ger)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla25
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
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

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

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Whiteboard: [good first bug][mentor=Ms2ger][lang=c++]

Comment 2

5 years ago
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
Created attachment 688253 [details] [diff] [review]
table.createTBody v1

Try results:
https://tbpl.mozilla.org/?tree=Try&rev=2a89c6831c65
Assignee: nobody → marc.jessome
Attachment #688253 - Flags: feedback?(Ms2ger)
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?
Created attachment 688287 [details]
Testcase1
(Assignee)

Comment 6

5 years ago
Could you upload a patch to implement just createTBody, as the others don't exist in the spec?
Created attachment 688795 [details] [diff] [review]
table.createTBody v2

Remove implementation of deleteTBody() as it is not in spec and remove implementation of getTBody().
Attachment #688253 - Attachment is obsolete: true
Attachment #688253 - Flags: feedback?(Ms2ger)
Attachment #688795 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 8

5 years ago
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.
Attachment #688795 - Flags: feedback?(Ms2ger) → feedback+
Created attachment 689237 [details]
Testcase v2
Attachment #688287 - Attachment is obsolete: true
Created attachment 689242 [details] [diff] [review]
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.
Attachment #688795 - Attachment is obsolete: true
Attachment #689242 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 11

5 years ago
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();
}
Created attachment 689793 [details] [diff] [review]
table.createTBody v4
Attachment #689242 - Attachment is obsolete: true
Attachment #689242 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 13

4 years ago
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.
(Assignee)

Updated

4 years ago
Flags: needinfo?(Ms2ger)
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.
(Assignee)

Comment 15

4 years ago
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.
Flags: needinfo?(Ms2ger)
(Assignee)

Comment 16

4 years ago
Stealing this; looking at writing tests now.
Assignee: marc.jessome → Ms2ger
(Assignee)

Updated

4 years ago
Whiteboard: [good first bug][mentor=Ms2ger][lang=c++]
(Assignee)

Comment 17

4 years ago
Created attachment 769329 [details] [diff] [review]
Patch v5

I updated this.
Attachment #689237 - Attachment is obsolete: true
Attachment #689793 - Attachment is obsolete: true
Attachment #769329 - Flags: review?(mounir)
(Assignee)

Updated

4 years ago
Flags: in-testsuite+
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?
Attachment #769329 - Flags: superreview?(bugs)
Attachment #769329 - Flags: review?(mounir)
Attachment #769329 - Flags: review+
(Assignee)

Comment 19

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

Updated

4 years ago
Attachment #769329 - Flags: superreview?(bugs) → superreview+
(Assignee)

Comment 20

4 years ago
https://hg.mozilla.org/mozilla-central/rev/33290309ae97
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.