1264947 - HTMLTableElement.rows doesn't seem to return element in the order defined in HTML Spec.

Status: RESOLVED FIXED

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

Description

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

If I read the code right, the implementation returns first elements in tbodies, and then children of table element itself. Per spec "followed by those elements whose parent is either a table or tbody element, again in tree order,"

Comment 1

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

Need to test what other browsers do.

Comment 2

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

Attachment: test

Comment 3

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

And we're behavior differently to Edge and Chrome.

I assume fixing this should be quite easy. Some changes to
#define DO_FOR_EACH_ROWGROUP(_code) in HTMLTableElement.cpp
We probably need separate macro/method to find certain element based on an index and then the existing macro works for a fast way to get the length of the collection.
Or something, could be fixed also in different way.

Comment 4

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

(In reply to Olli Pettay [:smaug] from comment #3)
> And we're behavior differently to Edge and Chrome.
huh, how did I write that bad English :)



Anyhow, link to the spec
https://html.spec.whatwg.org/multipage/tables.html#dom-table-rows

Comment 5

[John Dai[:jdai]]

Attachment: test2

Comment 6

[John Dai[:jdai]]

Attachment: Part1: Bug 1264947 - Remove redundant trailing spaces.

Comment 7

[John Dai[:jdai]]

Attachment: wip, v1

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6101289ce9d5783b55396fb0aa6d82cb753eab8

Comment 8

[John Dai[:jdai]]

Attachment: Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order.

Comment 9

[John Dai[:jdai]]

Comment on attachment 8750638 [details] [diff] [review]
Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order.

Base on comment #3, I separate a macro named DO_FOR_EACH_BY_ORDER()
 which is to find certain element based on an index.

Hi Olli, may I have your review? Thank you.

Comment 10

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

I presume we'll need to change http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/html/semantics/tabular-data/the-table-element/table-rows.html.ini with the patch. Can we even remove that whole .ini, assuming all the tests in http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/html/semantics/tabular-data/the-table-element/table-rows.html?force=1 are passed.

Comment 11

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

Looks like no, since that test fails because of some issue in head/foot handling. I wonder if that is issue in the test or in the spec or in our impl.

Comment 12

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

Comment on attachment 8750638 [details] [diff] [review]
Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order.

yeah, looks like we just handle the first thead and first tfoot, but per spec .rows should handle them all.
So the macro here cann't use mParent->GetTHead()/GetTFoot() but needs to actually iterate through all the child node for those.

And once that is done, the wpt test should pass and the .ini file which annotates known failures could be removed.

Comment 13

[John Dai[:jdai]]

Attachment: Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order. v2

Address comment 10, which are
- Make all the tests in table-rows.html are passed.
- Modify TableRowsCollection::GetLength() apply DO_FOR_EACH_BY_ORDER().
- Remove test_bug1264947.html because wpt table-rows.html cover all of them.

Try result:https://treeherder.mozilla.org/#/jobs?repo=try&revision=d914eb7fb451ee44831f032bec1e630dd921ac4a

Hi Olli, may I have your review? Thank you.

Comment 14

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

Now that I look at the code some more, do we ever need to use DO_FOR_EACH_ROWGROUP, or should we always use the new macro?
In other words, could or should TableRowsCollection::GetSupportedNames and TableRowsCollection::NamedItem use the new macro? I think so.

Comment 15

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #14)
> Now that I look at the code some more, do we ever need to use
> DO_FOR_EACH_ROWGROUP, or should we always use the new macro?
> In other words, could or should TableRowsCollection::GetSupportedNames and
> TableRowsCollection::NamedItem use the new macro? I think so.

Yes, you are right. We should use new macro. Thank you for pointing them out.
Looks like TableRowsCollection::NamedItem no method use it, perhaps I should add comment for it?

Comment 16

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

I'm pretty sure the generated code for HTMLCollection webidl interface uses NamedItem.

Comment 17

[John Dai[:jdai]]

Attachment: Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order. v3

Address comment 14, which is
- Apply TableRowsCollection::GetSupportedNames and TableRowsCollection::NamedItem to new macro.

Comment 18

[John Dai[:jdai]]

Attachment: Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order. v4

Following comment 17, I removed mOrphanRows in this patch.

Hi Olli, may I have your review? Thank you.

Comment 19

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

Comment on attachment 8752045 [details] [diff] [review]
Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order. v4

> // Macro that can be used to avoid copy/pasting code to iterate over the
> // rowgroups.  _code should be the code to execute for each rowgroup.  The
>-// rowgroup's rows will be in the nsIDOMHTMLCollection* named "rows".  Note
>-// that this may be null at any time.  This macro assumes an nsresult named
>+// rowgroup's rows will be in the nsIDOMHTMLCollection* named "rows".
>+// _trCode should be the code to excute for each tr row.  Note that
execute

>+// this may be null at any time.  This macro assumes an nsresult named
> // |rv| is in scope.
>-#define DO_FOR_EACH_ROWGROUP(_code)                                  \
>-  do {                                                               \
>-    if (mParent) {                                                   \
>-      /* THead */                                                    \
>-      HTMLTableSectionElement* rowGroup = mParent->GetTHead();       \
>-      nsIHTMLCollection* rows;                                       \
>-      if (rowGroup) {                                                \
>-        rows = rowGroup->Rows();                                     \
>-        do { /* gives scoping */                                     \
>-          _code                                                      \
>-        } while (0);                                                 \
>-      }                                                              \
>-      /* TBodies */                                                  \
>-      for (nsIContent* _node = mParent->nsINode::GetFirstChild();    \
>-           _node; _node = _node->GetNextSibling()) {                 \
>-        if (_node->IsHTMLElement(nsGkAtoms::tbody)) {                \
>-          rowGroup = static_cast<HTMLTableSectionElement*>(_node);   \
>-          rows = rowGroup->Rows();                                   \
>-          do { /* gives scoping */                                   \
>-            _code                                                    \
>-          } while (0);                                               \
>+  #define DO_FOR_EACH_BY_ORDER(_code, _trCode)                       \
>+    do {                                                             \
>+      if (mParent) {                                                 \
>+        HTMLTableSectionElement* rowGroup;                           \
>+        nsIHTMLCollection* rows;                                     \
>+        /* THead */                                                  \
>+        for (nsIContent* _node = mParent->nsINode::GetFirstChild();  \
>+             _node; _node = _node->GetNextSibling()) {               \
>+           if (_node->IsHTMLElement(nsGkAtoms::thead)) {             \
>+             rowGroup = static_cast<HTMLTableSectionElement*>(_node);\
>+             rows = rowGroup->Rows();                                \
>+             do { /* gives scoping */                                \
>+               _code                                                 \
>+             } while (0);                                            \
>+           }                                                         \
>+        }                                                            \
>+        /* TBodies */                                                \
>+        for (nsIContent* _node = mParent->nsINode::GetFirstChild();  \
>+             _node; _node = _node->GetNextSibling()) {               \
>+          if (_node->IsHTMLElement(nsGkAtoms::tr)) {                 \
>+            do {                                                     \
>+              _trCode                                                \
>+            } while (0);                                             \
>+          }                                                          \
>+          if (_node->IsHTMLElement(nsGkAtoms::tbody)) {              \
This 'if' could be 'else if', right?

> TableRowsCollection::GetFirstNamedElement(const nsAString& aName, bool& aFound)
> {
>   aFound = false;
>-  DO_FOR_EACH_ROWGROUP(
>+  DO_FOR_EACH_BY_ORDER({
>     Element* item = rows->NamedGetter(aName, aFound);
>     if (aFound) {
>       return item;
>     }
>-  );
>+  }, {
>+    nsCOMPtr<nsIAtom> name = NS_Atomize(aName);
>+    NS_ENSURE_TRUE(name, nullptr);
Perhaps move this atomize to be before the macro usage so that we don't repeat it for each <tr> elements.

>+    if (_node &&
>+        ((_node->IsHTMLElement() &&
Is there a need for _node->IsHTMLElement() check here. Don't we do that inside the macro since we have if (_node->IsHTMLElement(nsGkAtoms::tr)) { there.

>+          _node->AttrValueIs(kNameSpaceID_None, nsGkAtoms::name,
>+                               name, eCaseMatters)) ||
2 spaces too much before 'name'

>+          _node->AttrValueIs(kNameSpaceID_None, nsGkAtoms::id,
>+                               name, eCaseMatters))) {
ditto

> TableRowsCollection::GetSupportedNames(nsTArray<nsString>& aNames)
> {
>-  DO_FOR_EACH_ROWGROUP(
>+  AutoTArray<nsIAtom*, 8> atoms;
>+  DO_FOR_EACH_BY_ORDER({
>     nsTArray<nsString> names;
>     nsCOMPtr<nsIHTMLCollection> coll = do_QueryInterface(rows);
>     if (coll) {
>       coll->GetSupportedNames(names);
>       for (uint32_t i = 0; i < names.Length(); ++i) {
>         if (!aNames.Contains(names[i])) {
>           aNames.AppendElement(names[i]);
>         }
>       }
>     }
>-  );
>+  }, {
>+    if (_node->HasID()) {
>+      nsIAtom* id = _node->GetID();
>+      MOZ_ASSERT(id != nsGkAtoms::_empty,
>+                 "Empty ids don't get atomized");
>+      if (!atoms.Contains(id)) {
>+        atoms.AppendElement(id);
>+      }
>+    }
>+
>+    nsGenericHTMLElement* el = nsGenericHTMLElement::FromContent(_node);
>+    if (el) {
>+      const nsAttrValue* val = el->GetParsedAttr(nsGkAtoms::name);
>+      if (val && val->Type() == nsAttrValue::eAtom) {
>+        nsIAtom* name = val->GetAtomValue();
>+        MOZ_ASSERT(name != nsGkAtoms::_empty,
>+                   "Empty names don't get atomized");
>+        if (!atoms.Contains(name)) {
>+          atoms.AppendElement(name);
>+        }
>+      }
>+    }
>+
>+    uint32_t atomsLen = atoms.Length();
>+
>+    nsString* names = aNames.AppendElements(atomsLen);
>+    for (uint32_t i = 0; i < atomsLen; ++i) {
>+      atoms[i]->ToString(names[i]);
>+    }
What if aNames already contains the name, I mean what if it has been added there already in the block which deals with rows.
I think it would be simpler if you just played with strings here and not have separate array for atoms.
Note, if you need to convert atom to string for comparisons etc, nsDependentAtomString can be handy.


I'd like to see a new patch fixing TableRowsCollection::GetSupportedNames.

Comment 20

[John Dai[:jdai]]

Attachment: Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order. v5

Thanks for your useful information. I address it all and add a testcase in my patch.

Hi Olli, may I have your review? Thank you.

Comment 21

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

Comment on attachment 8752761 [details] [diff] [review]
Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order. v5

> TableRowsCollection::GetFirstNamedElement(const nsAString& aName, bool& aFound)
> {
>   aFound = false;
>-  DO_FOR_EACH_ROWGROUP(
>+  nsCOMPtr<nsIAtom> nameAtom = NS_Atomize(aName);
>+  DO_FOR_EACH_BY_ORDER({
>     Element* item = rows->NamedGetter(aName, aFound);
>     if (aFound) {
>       return item;
>     }
>-  );
>+  }, {
>+    NS_ENSURE_TRUE(nameAtom, nullptr);
move also this NS_ENSURE_TRUE to be next to NS_Atomize(aName);

>+++ b/dom/html/test/test_bug1264947.html
You could have added all the new tests to the existing web-platform-test, but up to you.

Comment 22

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8752761 [details] [diff] [review]
> Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order. v5
> 
> > TableRowsCollection::GetFirstNamedElement(const nsAString& aName, bool& aFound)
> > {
> >   aFound = false;
> >-  DO_FOR_EACH_ROWGROUP(
> >+  nsCOMPtr<nsIAtom> nameAtom = NS_Atomize(aName);
> >+  DO_FOR_EACH_BY_ORDER({
> >     Element* item = rows->NamedGetter(aName, aFound);
> >     if (aFound) {
> >       return item;
> >     }
> >-  );
> >+  }, {
> >+    NS_ENSURE_TRUE(nameAtom, nullptr);
> move also this NS_ENSURE_TRUE to be next to NS_Atomize(aName);
Will do.
> 
> >+++ b/dom/html/test/test_bug1264947.html
> You could have added all the new tests to the existing web-platform-test,
> but up to you.
I will put my tests into existing web-platform-test. Thank you.

Comment 23

[John Dai[:jdai]]

Attachment: Part1: Bug 1264947 - Remove redundant trailing spaces. (Final) r=smaug

Comment 24

[John Dai[:jdai]]

Attachment: Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order. (Final) r=smaug

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4fea5191a556bf31cfe9a22ed75d09fd8d44cfc

Comment 25

[John Dai[:jdai]]

Attachment: Part 2: Bug 1264947 - HTMLTableElement.rows return element in the order. (Final) r=smaug

I upload a new patch because the previous patch missed part of code.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4fea5191a556bf31cfe9a22ed75d09fd8d44cfc

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/533d90705f59
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec1046025657

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/533d90705f59
https://hg.mozilla.org/mozilla-central/rev/ec1046025657