Bug 148842 – Update HTMLTableRowElement.rowIndex to behave according to the specification

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

document.createElement("table").appendChild(document.createElement('tr')).rowIndex
should be identically equal to 0.

This bug was found by the newly added test:
LayoutTests/http/tests/w3c/html/semantics/tabular-data/the-tr-element/rowIndex.html

[Radar WebKit Bug Importer]

<rdar://problem/22586914>

[Chris Dumez]

Created attachment 261702 [details]
Patch

[Darin Adler]

Comment on attachment 261702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261702&action=review

> Source/WebCore/ChangeLog:25
> +        The code is refactored a bit to leverage HTMLTableElement.rows(), as
> +        per the specification:
> +        "return the index of the tr element in that table element's rows collection"
> +
> +        Previously, we would duplicate the table traversal logic. This
> +        simplifies the code a bit.

Is the new implementation more or less efficient than the old one? I understand that it’s simpler.

> Source/WebCore/html/HTMLTableRowElement.cpp:59
> +    auto* parent = parentNode();

Maybe consider using parentElement here instead of parentNode.

> Source/WebCore/html/HTMLTableRowElement.cpp:73
> +    unsigned rowCount = rows->length();

I would have named this local variable "length".

[Darin Adler]

Comment on attachment 261702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261702&action=review

> Source/WebCore/ChangeLog:3
> +        tr.rowIndex should return 0 when tr is the only direct child of a table element

I don’t see any tests that match this description of the bug. The one test changing its result is not about rowIndex returning 0.

[Build Bot]

Comment on attachment 261702 [details]
Patch

Attachment 261702 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/194779

New failing tests:
fast/table/rowindex.html

[Build Bot]

Created attachment 261705 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 261702 [details]
Patch

Attachment 261702 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/194809

New failing tests:
fast/table/rowindex.html

[Build Bot]

Created attachment 261707 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 261713 [details]
Patch

[Chris Dumez]

Comment on attachment 261702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261702&action=review

>> Source/WebCore/ChangeLog:3
>> +        tr.rowIndex should return 0 when tr is the only direct child of a table element
> 
> I don’t see any tests that match this description of the bug. The one test changing its result is not about rowIndex returning 0.

It is though. We filed this bug specifically because of this test that was failing. The test looks like this:
"""
test(function() {
  var row = document.createElement("table")
                    .appendChild(document.createElement("tr"));
  assert_equals(row.rowIndex, 0);
});
"""

It just doesn't show in the -expected.txt.

>> Source/WebCore/ChangeLog:25
>> +        simplifies the code a bit.
> 
> Is the new implementation more or less efficient than the old one? I understand that it’s simpler.

It should be equivalent in terms of performance. If performance is a problem, I am sure we can make HTMLTableRowsCollection a bit faster.

>> Source/WebCore/html/HTMLTableRowElement.cpp:59
>> +    auto* parent = parentNode();
> 
> Maybe consider using parentElement here instead of parentNode.

Not the first time we discuss this but parentElement() will do an unnecessary isElementNode() check. Also, knowing the parent is an Element is not going to make the is<HTMLTableElement>() check any faster.

>> Source/WebCore/html/HTMLTableRowElement.cpp:73
>> +    unsigned rowCount = rows->length();
> 
> I would have named this local variable "length".

Ok.

[Chris Dumez]

Created attachment 261714 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 261714 [details]
Patch

Clearing flags on attachment: 261714

Committed r190099: <http://trac.webkit.org/changeset/190099>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This made fast/table/rowindex.html fail on Windows. Chris, could you please take a look?

[Darin Adler]

Comment on attachment 261714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261714&action=review

> LayoutTests/ChangeLog:12
> +        * platform/mac/fast/table/rowindex-expected.txt:

Looks like there are also many other rowindex-expected files:

platform/efl/fast/table/rowindex-expected.png
platform/efl/fast/table/rowindex-expected.txt
platform/gtk/fast/table/rowindex-expected.png
platform/gtk/fast/table/rowindex-expected.txt
platform/ios-simulator/fast/table/rowindex-expected.txt
platform/ios-simulator-wk2/fast/table/rowindex-expected.txt
platform/mac/fast/table/rowindex-expected.png
platform/win/fast/table/rowindex-expected.txt

What should we do about all of those!?

[Chris Dumez]

(In reply to comment #15)
> Comment on attachment 261714 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261714&action=review
> 
> > LayoutTests/ChangeLog:12
> > +        * platform/mac/fast/table/rowindex-expected.txt:
> 
> Looks like there are also many other rowindex-expected files:
> 
> platform/efl/fast/table/rowindex-expected.png
> platform/efl/fast/table/rowindex-expected.txt
> platform/gtk/fast/table/rowindex-expected.png
> platform/gtk/fast/table/rowindex-expected.txt
> platform/ios-simulator/fast/table/rowindex-expected.txt
> platform/ios-simulator-wk2/fast/table/rowindex-expected.txt
> platform/mac/fast/table/rowindex-expected.png
> platform/win/fast/table/rowindex-expected.txt
> 
> What should we do about all of those!?

I will rebaseline them.

[Chris Dumez]

(In reply to comment #16)
> (In reply to comment #15)
> > Comment on attachment 261714 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=261714&action=review
> > 
> > > LayoutTests/ChangeLog:12
> > > +        * platform/mac/fast/table/rowindex-expected.txt:
> > 
> > Looks like there are also many other rowindex-expected files:
> > 
> > platform/efl/fast/table/rowindex-expected.png
> > platform/efl/fast/table/rowindex-expected.txt
> > platform/gtk/fast/table/rowindex-expected.png
> > platform/gtk/fast/table/rowindex-expected.txt
> > platform/ios-simulator/fast/table/rowindex-expected.txt
> > platform/ios-simulator-wk2/fast/table/rowindex-expected.txt
> > platform/mac/fast/table/rowindex-expected.png
> > platform/win/fast/table/rowindex-expected.txt
> > 
> > What should we do about all of those!?
> 
> I will rebaseline them.

<http://trac.webkit.org/changeset/190116>