1335356 - Issues with HTMLTableElement caption members

Status: RESOLVED FIXED

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

Description

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

http://w3c-test.org/html/semantics/tabular-data/the-table-element/caption-methods.html has a bunch of failures.

In particular, it looks like we don't insert a new caption as the first child of the table with createCaption() and when setting table.caption.

Also, createCaption() will use the table's namespace prefix if it has one; the spec doesn't say that. (Though perhaps it could be argued that it should? Chrome follows the spec.)

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Jessica, could you please take a look at this? Thanks!

Comment 2

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> Hi Jessica, could you please take a look at this? Thanks!

Sure.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Fixes order of insertion for caption and thead.

Comment 4

[Jessica Jong [:jessica] (inactive)]

(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #0)
> 
> Also, createCaption() will use the table's namespace prefix if it has one;
> the spec doesn't say that. (Though perhaps it could be argued that it
> should? Chrome follows the spec.)

Anne, in Firefox, all elements of a table: caption, thead, tfoot, etc., use the table's namespace prefix, but the spec does not mention this. Do you think the spec should be updated? or..?

Comment 5

[Anne (:annevk)]

Given that Safari passes all tests, and Chrome all minus one, I think we should just fix our implementation for these methods. I tried testing Edge, but it seemingly crashes on this test. I filed https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10980319/ on that.

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to Anne (:annevk) from comment #5)
> Given that Safari passes all tests, and Chrome all minus one, I think we
> should just fix our implementation for these methods. I tried testing Edge,
> but it seemingly crashes on this test. I filed
> https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/
> 10980319/ on that.

Thanks. Will upload a new patch to fix our implementation.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8838404 [details] [diff] [review]
patch, v2.

In nsContentUtils::NameChanged when getting a new NodeInfo (only name is changed), use a null prefix instead.

Places that use nsContentUtils::NameChanged:
- HTMLTableElement::CreateTHead
- HTMLTableElement::CreateTFoot
- HTMLTableElement::CreateCaption
- HTMLTableElement::InsertRow
- HTMLTableRowElement::InsertCell
- HTMLTableSectionElement::InsertRow
- HTMLSelectElement::SetLength

Most of them are when creating child element of a table, so I think it's safe to set it to null.

Comment 9

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

Now you get to write tests for all those callers :)

Comment 10

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

Comment on attachment 8838404 [details] [diff] [review]
patch, v2.


>diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp
>--- a/dom/base/nsContentUtils.cpp
>+++ b/dom/base/nsContentUtils.cpp
>@@ -3443,17 +3443,18 @@ nsContentUtils::IsDraggableLink(const ns
> 
> // static
> nsresult
> nsContentUtils::NameChanged(mozilla::dom::NodeInfo* aNodeInfo, nsIAtom* aName,
>                             mozilla::dom::NodeInfo** aResult)
> {
>   nsNodeInfoManager *niMgr = aNodeInfo->NodeInfoManager();
> 
>-  *aResult = niMgr->GetNodeInfo(aName, aNodeInfo->GetPrefixAtom(),
>+  // Prefix should be null for new NodeInfo.
>+  *aResult = niMgr->GetNodeInfo(aName, nullptr,
>                                 aNodeInfo->NamespaceID(),
>                                 aNodeInfo->NodeType(),
>                                 aNodeInfo->GetExtraName()).take();
So, this method is called in many places. Did you verify they all start to work per spec?
And I think the method name is now wrong. It should hint that prefix becomes null.
Perhaps QNameChanged, and then document in .h that prefix becomes null.




> HTMLTableElement::DeleteTHead()
> {
>@@ -470,17 +481,19 @@ HTMLTableElement::CreateCaption()
>     nsContentUtils::NameChanged(mNodeInfo, nsGkAtoms::caption,
>                                 getter_AddRefs(nodeInfo));
> 
>     caption = NS_NewHTMLTableCaptionElement(nodeInfo.forget());
>     if (!caption) {
>       return nullptr;
>     }
> 
>-    AppendChildTo(caption, true);
>+    ErrorResult rv;
>+    nsCOMPtr<nsINode> firsChild = nsINode::GetFirstChild();
>+    nsINode::InsertBefore(*caption, firsChild, rv);

You want to use IgnoredErrorResult, or add [Throws] to the .webidl and pass the aRv here.
Looks like CreateTHead has similar code. I think using IgnoredErrorResult should be fine, since I assume the only
possible error is OOM, and we should crash when that happens.


>     DeleteTHead();
>     if (aTHead) {
>-      nsCOMPtr<nsINode> refNode = nsINode::GetFirstChild();
>+
>+      nsCOMPtr<nsIContent> refNode = nullptr;
>+      for (refNode = nsINode::GetFirstChild();
>+           refNode;
>+           refNode = refNode->GetNextSibling()) {
>+        if (refNode->IsHTMLElement() &&
>+            !refNode->IsHTMLElement(nsGkAtoms::caption) &&
>+            !refNode->IsHTMLElement(nsGkAtoms::colgroup)) {
>+          break;
>+        }
>+      }
>+
Spec wording is rather weird with setting thead, but I interpret it this way too

r- because of NameChanged. Need to verify the changed behavior is expected by the callers, and if not, 
add a new QNameChange or some such and use that when it is ok to use it.
If the new behavior is ok for all the callers, still rename NameChanged

Comment 11

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #10)
> Comment on attachment 8838404 [details] [diff] [review]
> patch, v2.
> 
> 
> >diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp
> >--- a/dom/base/nsContentUtils.cpp
> >+++ b/dom/base/nsContentUtils.cpp
> >@@ -3443,17 +3443,18 @@ nsContentUtils::IsDraggableLink(const ns
> > 
> > // static
> > nsresult
> > nsContentUtils::NameChanged(mozilla::dom::NodeInfo* aNodeInfo, nsIAtom* aName,
> > mozilla::dom::NodeInfo** aResult)
> > {
> > nsNodeInfoManager *niMgr = aNodeInfo->NodeInfoManager();
> > 
> >- *aResult = niMgr->GetNodeInfo(aName, aNodeInfo->GetPrefixAtom(),
> >+ // Prefix should be null for new NodeInfo.
> >+ *aResult = niMgr->GetNodeInfo(aName, nullptr,
> > aNodeInfo->NamespaceID(),
> > aNodeInfo->NodeType(),
> > aNodeInfo->GetExtraName()).take();
> So, this method is called in many places. Did you verify they all start to
> work per spec?
> And I think the method name is now wrong. It should hint that prefix becomes
> null.
> Perhaps QNameChanged, and then document in .h that prefix becomes null.
> 
> 
> 
> 
> > HTMLTableElement::DeleteTHead()
> > {
> >@@ -470,17 +481,19 @@ HTMLTableElement::CreateCaption()
> > nsContentUtils::NameChanged(mNodeInfo, nsGkAtoms::caption,
> > getter_AddRefs(nodeInfo));
> > 
> > caption = NS_NewHTMLTableCaptionElement(nodeInfo.forget());
> > if (!caption) {
> > return nullptr;
> > }
> > 
> >- AppendChildTo(caption, true);
> >+ ErrorResult rv;
> >+ nsCOMPtr<nsINode> firsChild = nsINode::GetFirstChild();
> >+ nsINode::InsertBefore(*caption, firsChild, rv);
> 
> You want to use IgnoredErrorResult, or add [Throws] to the .webidl and pass
> the aRv here.
> Looks like CreateTHead has similar code. I think using IgnoredErrorResult
> should be fine, since I assume the only
> possible error is OOM, and we should crash when that happens.
> 

Will change to IgnoredErrorResult, here and elsewhere.

> 
> > DeleteTHead();
> > if (aTHead) {
> >- nsCOMPtr<nsINode> refNode = nsINode::GetFirstChild();
> >+
> >+ nsCOMPtr<nsIContent> refNode = nullptr;
> >+ for (refNode = nsINode::GetFirstChild();
> >+ refNode;
> >+ refNode = refNode->GetNextSibling()) {
> >+ if (refNode->IsHTMLElement() &&
> >+ !refNode->IsHTMLElement(nsGkAtoms::caption) &&
> >+ !refNode->IsHTMLElement(nsGkAtoms::colgroup)) {
> >+ break;
> >+ }
> >+ }
> >+
> Spec wording is rather weird with setting thead, but I interpret it this way
> too
> 
> r- because of NameChanged. Need to verify the changed behavior is expected
> by the callers, and if not, 
> add a new QNameChange or some such and use that when it is ok to use it.
> If the new behavior is ok for all the callers, still rename NameChanged

As mentioned in comment 8, most of the NameChanged callers use this method when creating child elements of a table. The spec does not't mention using the table's prefix, so using a null prefix seems fine. For 'HTMLSelectElement::SetLength', it is called when the length set is larger than the current length, so adding dummy options should be like creating new HTMLSelectElements.

Hence, I'll just change the method name to QNameChanged and add some comments for it. Will also add some wpts for this.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: Insert caption and thead in the right order.

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Rename nsContentUtils::NameChanged to QNameChanged and use null prefix.

Comment 14

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

Comment on attachment 8839342 [details] [diff] [review]
Part 2: Rename nsContentUtils::NameChanged to QNameChanged and use null prefix.

>+        assert_equals(child.prefix, null,
>+                      "new child should not copy select's prefix");
You ensured that other browsers have this behavior?
The spec doesn't actually say it explicitly whether there should be prefix - so I guess it means there shouldn't be.

>+    assert_equals(tfoot.prefix, null);
ditto

>+    assert_equals(thead.prefix, null);
and this too.

Comment 15

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #14)
> Comment on attachment 8839342 [details] [diff] [review]
> Part 2: Rename nsContentUtils::NameChanged to QNameChanged and use null
> prefix.
> 
> >+        assert_equals(child.prefix, null,
> >+                      "new child should not copy select's prefix");
> You ensured that other browsers have this behavior?
> The spec doesn't actually say it explicitly whether there should be prefix -
> so I guess it means there shouldn't be.
> 
> >+    assert_equals(tfoot.prefix, null);
> ditto
> 
> >+    assert_equals(thead.prefix, null);
> and this too.

Yes, I tested on Chrome, Safari and Edge with https://jsfiddle.net/jessi3py/c48z8nee/, and they all have a null prefix. Also confirmed with Anne in comment 5.

Comment 16

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcdabc07440c98399847601fe56e95341f69b20b&group_state=expanded

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f882927cb0
Part 1: Insert caption and thead in the right order. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b78f38eef9e
Part 2: Rename nsContentUtils::NameChanged to QNameChanged and use null prefix. r=smaug

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b4f882927cb0
https://hg.mozilla.org/mozilla-central/rev/9b78f38eef9e