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

VERIFIED FIXED in Firefox 32

Status

()

defect
VERIFIED FIXED
5 years ago
2 months ago

People

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

Tracking

({dev-doc-needed, site-compat})

32 Branch
mozilla32
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32 verified)

Details

(Whiteboard: [good first bug][mentor=bzbarsky@mit.edu][lang=c++])

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

5 years ago
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++]
Assignee

Comment 2

5 years ago
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.").
Assignee

Comment 3

5 years ago
My first C++ patch to a free software
Assignee

Comment 4

5 years ago
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!
Assignee

Updated

5 years ago
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+
Assignee

Comment 8

5 years ago
Remove temporary variables, and use child->IsHTML instead.
Attachment #8415734 - Attachment is obsolete: true
Attachment #8416076 - Flags: review?(bzbarsky)
Assignee

Comment 9

5 years ago
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.
Assignee

Comment 11

5 years ago
Shame on me :)
Attachment #8416076 - Attachment is obsolete: true
Attachment #8416076 - Flags: review?(bzbarsky)
Attachment #8416094 - Flags: review?(bzbarsky)
Assignee

Comment 12

5 years ago
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+
Assignee

Comment 15

5 years ago
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
Assignee

Comment 21

5 years ago
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
Last Resolved: 5 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]

Comment 24

5 years ago
[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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.