Issues with HTMLTableElement caption members

RESOLVED FIXED in Firefox 54

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Ms2ger, Assigned: jessica)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.)
Hi Jessica, could you please take a look at this? Thanks!
Flags: needinfo?(jjong)
Priority: -- → P2
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> Hi Jessica, could you please take a look at this? Thanks!

Sure.
Assignee: nobody → jjong
Flags: needinfo?(jjong)
Posted patch patch, v1. (obsolete) — Splinter Review
Fixes order of insertion for caption and thead.
(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..?
Flags: needinfo?(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.
Flags: needinfo?(annevk)
(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.
Posted patch patch, v2. (obsolete) — Splinter Review
Attachment #8837941 - Attachment is obsolete: true
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.
Attachment #8838404 - Flags: review?(bugs)
Now you get to write tests for all those callers :)
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
Attachment #8838404 - Flags: review?(bugs) → review-
(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.
Attachment #8839341 - Flags: review?(bugs)
Attachment #8839342 - Flags: review?(bugs)
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.
Attachment #8839342 - Flags: review?(bugs) → review+
Attachment #8839341 - Flags: review?(bugs) → review+
(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.
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b4f882927cb0
https://hg.mozilla.org/mozilla-central/rev/9b78f38eef9e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.