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)
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)
4.47 KB,
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
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•10 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•10 years ago
|
||
My first C++ patch to a free software
Assignee | ||
Comment 4•10 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)
Comment 5•10 years ago
|
||
(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?
Comment 6•10 years ago
|
||
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•10 years ago
|
Attachment #8415734 -
Flags: review?(bzbarsky)
Comment 7•10 years ago
|
||
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•10 years ago
|
||
Remove temporary variables, and use child->IsHTML instead.
Attachment #8415734 -
Attachment is obsolete: true
Attachment #8416076 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•10 years ago
|
||
Ok, what does "r=me" mean ? And how do I add a test? Which kind of test?
Comment 10•10 years ago
|
||
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•10 years ago
|
||
Shame on me :)
Attachment #8416076 -
Attachment is obsolete: true
Attachment #8416076 -
Flags: review?(bzbarsky)
Attachment #8416094 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 12•10 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?
Comment 13•10 years ago
|
||
It's not. Just ignore the "try to avoid" BS. I wish it weren't there. :(
Comment 14•10 years ago
|
||
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•10 years ago
|
||
Add a mochitest test.
Attachment #8416094 -
Attachment is obsolete: true
Attachment #8416137 -
Flags: review?(bzbarsky)
Comment 16•10 years ago
|
||
Comment on attachment 8416137 [details] [diff] [review] bug-1003539-add-row-to-last-tbody.patch Looks wonderful.
Attachment #8416137 -
Flags: review?(bzbarsky) → review+
Comment 17•10 years ago
|
||
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
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
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. ;)
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Attachment #8416137 -
Attachment is obsolete: true
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/864680954235
Keywords: checkin-needed
Assignee | ||
Comment 21•10 years ago
|
||
Thank you for your help and your support! I'm very happy to have contributed to such a beautiful project.
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/864680954235
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 23•10 years ago
|
||
We're very happy you contributed as well!
Updated•10 years ago
|
Updated•10 years ago
|
QA Whiteboard: [good first verify]
Comment 24•10 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.
Comment 25•10 years ago
|
||
(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]
status-firefox32:
--- → verified
Updated•10 years ago
|
Flags: in-testsuite+
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•