Bug 148842

Summary: Update HTMLTableRowElement.rowIndex to behave according to the specification
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://html.spec.whatwg.org/multipage/tables.html#dom-tr-rowindex
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews102 for mac-mavericks
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
Patch
none
Patch none

Description Ryosuke Niwa 2015-09-04 16:16:42 PDT
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
Comment 1 Radar WebKit Bug Importer 2015-09-04 16:17:01 PDT
<rdar://problem/22586914>
Comment 2 Chris Dumez 2015-09-21 16:38:06 PDT
Created attachment 261702 [details]
Patch
Comment 3 Darin Adler 2015-09-21 16:42:16 PDT
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".
Comment 4 Darin Adler 2015-09-21 16:42:55 PDT
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.
Comment 5 Build Bot 2015-09-21 17:10:42 PDT
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
Comment 6 Build Bot 2015-09-21 17:10:46 PDT
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
Comment 7 Build Bot 2015-09-21 17:22:44 PDT
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
Comment 8 Build Bot 2015-09-21 17:22:48 PDT
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
Comment 9 Chris Dumez 2015-09-21 18:50:50 PDT
Created attachment 261713 [details]
Patch
Comment 10 Chris Dumez 2015-09-21 18:58:29 PDT
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.
Comment 11 Chris Dumez 2015-09-21 19:11:27 PDT
Created attachment 261714 [details]
Patch
Comment 12 WebKit Commit Bot 2015-09-21 19:58:03 PDT
Comment on attachment 261714 [details]
Patch

Clearing flags on attachment: 261714

Committed r190099: <http://trac.webkit.org/changeset/190099>
Comment 13 WebKit Commit Bot 2015-09-21 19:58:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Alexey Proskuryakov 2015-09-21 23:07:13 PDT
This made fast/table/rowindex.html fail on Windows. Chris, could you please take a look?
Comment 15 Darin Adler 2015-09-22 08:24:42 PDT
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!?
Comment 16 Chris Dumez 2015-09-22 09:03:41 PDT
(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.
Comment 17 Chris Dumez 2015-09-22 09:11:47 PDT
(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>