Fix off-by-one in deleteCell and deleteRow index checks #1890
Conversation
|
This already has test coverage in https://github.com/w3c/web-platform-tests/blob/master/html/semantics/tabular-data/the-tr-element/deleteCell.html ("HTMLTableRowElement deleteCell(cells.length)") |
|
To be clear, deleteRow already says "greater than or equal to" and doesn't need changing, right? |
| <p>If the given position is less than −1 or greater than the index of the last cell, or | ||
| if there are no cells, throws an <span>"<code>IndexSizeError</code>"</span> | ||
| <p>If the given position is less than −1 or greater than or equal to the index of the | ||
| last cell, or if there are no cells, throws an <span>"<code>IndexSizeError</code>"</span> |
There was a problem hiding this comment.
Actually I think this part of the change is wrong. If there is only one cell, last cell index will be 0 and deleteCell(0) should be safe to do.
There was a problem hiding this comment.
Good catch, I'll align the wording with the below. Can I ack you as "Rob Buis"?
There was a problem hiding this comment.
Actually, two other dom intro blocks share the "index of the last x" wording, so I'll just keep it.
| <li><p>If <var>index</var> is less than −1 or greater than the number of elements in | ||
| the <code data-x="dom-tr-cells">cells</code> collection, then throw an | ||
| <li><p>If <var>index</var> is less than −1 or greater than or equal to the number of | ||
| elements in the <code data-x="dom-tr-cells">cells</code> collection, then throw an |
There was a problem hiding this comment.
This still seems correct, if there is 1 cell, index equal to number of elements is 1, and deleteCell(1) should be "out of range".
|
https://html.spec.whatwg.org/#dom-tbody-deleterow seems to have the same problem: |
domenic
added
the
do not merge yet
|
Sorry @rwlbuis, I didn't see your comments 6 days ago, must have accidentally marked some mail as read. |
|
Well this is interesting, per current spec |
|
Per the testing in web-platform-tests/wpt#4001 I think the best outcome is for |
This was likely copied from insertCell, where it makes sense. Discovered when reviewing an implementation change: https://codereview.chromium.org/2406423004/
foolip
force-pushed
the
deleteCell-off-by-one
branch
from
473c733
to
bc3e536
Compare
foolip
changed the title
|
That's #1924 |
foolip
removed
the
do not merge yet
|
Still no new tests required for this, the three files touched by web-platform-tests/wpt#4001 have tests in the style of "HTMLTableRowElement deleteCell(cells.length)" for all three cases. |
|
@zcorpan care to review? |
| @@ -120102,6 +120102,7 @@ INSERT INTERFACES HERE | |||
| Rikkert Koppes, | |||
| Rimantas Liubertas, | |||
| Riona Macnamara, | |||
| Rob Buis, | |||
|
Thanks @rwlbuis, I'll take that as review and am merging now. |
This was likely copied from insertCell, where it makes sense.
Discovered when reviewing an implementation change:
https://codereview.chromium.org/2406423004/