Closed Bug 1003539 Opened 10 years ago Closed 10 years ago

HTMLTableElement.insertRow doesn't insert the row at the right place when table has a thead or tfoot, no tbody, and no rows

Categories

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

32 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox32 --- verified

People

(Reporter: pere.jobs, Assigned: pere.jobs)

Details

(Keywords: dev-doc-needed, site-compat, Whiteboard: [good first bug][mentor=bzbarsky@mit.edu][lang=c++])

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140410211200

Steps to reproduce:

Enter console, type 
t = document.createElement('table');
thead = document.createElement('thead');
t.appendChild(thead);
t.insertRow();
console.log(t.innerHTML)


Actual results:

The result is "<thead><tr></tr></thead>" (the row is appended to the THEAD)


Expected results:

Chromium returns "<thead></thead><tbody><tr></tr></tbody>".
I'd expect the row not to be appended to the head.
Hmm.  In DOM 2 core, I think our current behavior was in fact correct.  It looks like HTML5 changed from that behavior for some reason.

In any case, I think all that's needed here is to change the "find the first row group" loop in insertRow in the count == 0 case from looking for any table section to looking for a tbody only.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: HTMLTableElement.insertRow doesn't insert the row at the right place → HTMLTableElement.insertRow doesn't insert the row at the right place when table has a thead or tfoot, no tbody, and no rows
Whiteboard: [good first bug][mentor=bzbarsky@mit.edu][lang=c++]
I’ve never contributed to Firefox, nor am I an experimented cpp programmer, but as you marked this as a "good first bug", I started looking at the code.

First of all, is this the specification HTMLTableElement::InsertRow has to follow: http://www.whatwg.org/specs/web-apps/current-work/multipage/tabular-data.html#dom-table-insertrow ?

If it is, then I can see another bug: when there are several TBODYs, this function inserts the new row to the first one, instead of the last one. (In the spec: "The method must create a tr element, append it to the last tbody element in the table, and return the tr element.").
My first C++ patch to a free software
I downloaded the source to a directory that contains special characters, and I get:
Traceback (most recent call last):
  File "./mach", line 56, in <module>
    main(sys.argv)
  File "./mach", line 28, in main
    mozinfo_path = os.path.join(dir_path, 'mozinfo.json')
  File "/usr/lib/python2.7/posixpath.py", line 80, in join
    path += '/' + b
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 16: ordinal not in range(128)
(In reply to pere.jobs from comment #4)
> I downloaded the source to a directory that contains special characters, and
> I get:
> Traceback (most recent call last):
>   File "./mach", line 56, in <module>
>     main(sys.argv)
>   File "./mach", line 28, in main
>     mozinfo_path = os.path.join(dir_path, 'mozinfo.json')
>   File "/usr/lib/python2.7/posixpath.py", line 80, in join
>     path += '/' + b
> UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 16:
> ordinal not in range(128)

Could you please file that as a new bug under Core :: mach?
With regards to the patch, please visit the Details page and set the review flag to ?, then choose :bz as the target. Thanks for your contribution!
Attachment #8415734 - Flags: review?(bzbarsky)
Comment on attachment 8415734 [details] [diff] [review]
bug-1003539-add-row-to-last-tbody.patch

Thank you for the patch!

You should just be able to use child->IsHTML(nsGkAtoms::tbody) and nix the childInfo and localName temporaries.

r=me with that, but please add a test?
Attachment #8415734 - Flags: review?(bzbarsky) → review+
Remove temporary variables, and use child->IsHTML instead.
Attachment #8415734 - Attachment is obsolete: true
Attachment #8416076 - Flags: review?(bzbarsky)
Ok, what does "r=me" mean ?
And how do I add a test? Which kind of test?
Comment on attachment 8416076 [details] [diff] [review]
bug-1003539-add-row-to-last-tbody-add-isHTML.patch

"r=me" means review granted by me.  ;)  That said, you want to remove the "childInfo->NamespaceID() == kNameSpaceID_XHTML &&" bit, since it wouldn't compile anyway with the childInfo temporary removed, right?  IsHTML() already does a namespace check.

As far as tests go, https://developer.mozilla.org/en-US/docs/Mochitest#Writing_tests and https://developer.mozilla.org/en-US/docs/Mochitest#Adding_tests_to_the_tree have some directions.  If that sounds too painful, I can do the test bit; just let me know.
Shame on me :)
Attachment #8416076 - Attachment is obsolete: true
Attachment #8416076 - Flags: review?(bzbarsky)
Attachment #8416094 - Flags: review?(bzbarsky)
Thank you your reviews, and for taking the time to explain me everything whereas you could have fixed all this in 1 minute.

The first heading of  https://developer.mozilla.org/en-US/docs/Mochitest#Writing_tests is "Try to avoid Mochitest". Do you think it's possible to write an xpcshell test for this bug?
It's not.  Just ignore the "try to avoid" BS.  I wish it weren't there.  :(
Comment on attachment 8416094 [details] [diff] [review]
bug-1003539-add-row-to-last-tbody-add-isHTML.patch

This looks great.  The commit message should probably just say "r=bzbarsky" at the end.  ;)
Attachment #8416094 - Flags: review?(bzbarsky) → review+
Add a mochitest test.
Attachment #8416094 - Attachment is obsolete: true
Attachment #8416137 - Flags: review?(bzbarsky)
Comment on attachment 8416137 [details] [diff] [review]
bug-1003539-add-row-to-last-tbody.patch

Looks wonderful.
Attachment #8416137 - Flags: review?(bzbarsky) → review+
I kicked off a test run at https://tbpl.mozilla.org/?tree=Try&rev=092407027f90 to make sure we didn't have any other tests depending on the old behavior.  Once the results from that come in, I'll get the patch landed.

Thank you again for working on this!
Assignee: nobody → pere.jobs
There are two test failures, of a sort:

6454 ERROR TEST-UNEXPECTED-PASS | /tests/dom/imptests/html/html/semantics/tabular-data/the-table-element/test_table-insertRow.html | insertRow should insert into a tbody, not into a thead, if table.rows is empty
6455 ERROR TEST-UNEXPECTED-PASS | /tests/dom/imptests/html/html/semantics/tabular-data/the-table-element/test_table-insertRow.html | insertRow should insert into a tbody, not into a tfoot, if table.rows is empty

We just need to remove those known-fail annotations.  ;)
Attachment #8416137 - Attachment is obsolete: true
Thank you for your help and your support!
I'm very happy to have contributed to such a beautiful project.
https://hg.mozilla.org/mozilla-central/rev/864680954235
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
We're very happy you contributed as well!
OS: Linux → All
Hardware: x86_64 → All
QA Whiteboard: [good first verify]
[testday-20140814]

Verified on Windows 7 x64. Now, in Firefox 32.0b6, I'm getting the expected result, which is not the case in Firefox 31.
(In reply to kenkon from comment #24)
> [testday-20140814]
> 
> Verified on Windows 7 x64. Now, in Firefox 32.0b6, I'm getting the expected
> result, which is not the case in Firefox 31.

Marking as Verified. Thank you kenkon!
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify][testday-20140814]
Flags: in-testsuite+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.