188645 - insertRow(-1) work as insertRow(rowCount-1) for HTMLTableElement.cpp

Status: RESOLVED FIXED

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

Description

[Ruslan Ismailov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030110
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030110

When I wrote answer to bug 188635, I notice, that IncertRow(-1) add row not to
the end of table, but as rowCount-1. Look at testcase.

I check it, and source of this behaviour is error in nsHTMLTableElement.cpp, in
line 801:

800 // the index is greater than the number of rows, so just append
801       if ((0 <= aIndex) && (PRInt32(rowCount) <= aIndex)) {
802         rv = parent->AppendChild(newRowNode, getter_AddRefs(retChild));
803       }
804       else
805       {
806         // insert the new row before the reference row we found above
807         rv = parent->InsertBefore(newRowNode, refRow,
808                                   getter_AddRefs(retChild));
809       }

This If must distinguish append and insert cases, but it works only when
aIndex>=rowCount. If aIndex==-1, then new row inserted to a refIndex position,
and RefIndex=rowCount-1 for aIndex==-1 (line 778).

So I suggest to change line 801 to

801       if (((0 <= aIndex) && (PRInt32(rowCount) <= aIndex)) || (aIndex == -1)) {

Reproducible: Always

Steps to Reproduce:
1. Open testcase
2. Play with buttons -- they left do insertRow(-1), right insertRow(rowCount)
3. Notice the difference

Actual Results:  
insertRow(-1) is equal to insertRow(rowCount-1) 

Expected Results:  
insertRow(-1) should be equal to insertRow(rowCount)

Comment 1

[Ruslan Ismailov]

Attachment: insertRow(-1) work as insertRow(rowCount-1)

Comment 2

[Ruslan Ismailov]

Attachment: Proposed patch

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 111290 [details] [diff] [review]
Proposed patch

>       // the index is greater than the number of rows, so just append
>-      if ((0 <= aIndex) && (PRInt32(rowCount) <= aIndex)) {
>+      if (((0 <= aIndex) && (PRInt32(rowCount) <= aIndex)) 
>+          || (aIndex == -1)) {

Actually, you are guaranteed (by the spec) that by the time you get here, -1 <=
aIndex <= rowCount.  Note that the comment is wrong.  the index CANNOT be
greater than the number of rows.  If it is, we throw an exception (the DOM
specification demands it).  The spec[1] says "If index is -1 or equal to the
number of rows, the new row is appended.".  Please update the comment to copy
the spec, as well as make the code do that.

Note, that I know you are not at fault for the old code, but if you are going
to be complaining about spec stuff, please make it be accurate and correct. 
:-)


[1]
http://www.w3.org/TR/2003/REC-DOM-Level-2-HTML-20030109/html.html#ID-39872903

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

You want the following:

      // If index is -1 or equal to the number of rows, the new row
      // is appended.
      if (aIndex == -1 || PRUint32(aIndex) == rowCount) {

Comment 5

[Ruslan Ismailov]

Oops. I see that if aIndex > rowCount, than old code does not made an exeption,
simply add a row. Is it correct bahaviour? 

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

We do throw the exception.  See line 766 (and also 754).  So again, by the time
you get there, you know for a fact that it is between -1 and the rowCount,
inclusively.  All you need to do is check against the two endpoints.

Comment 7

[Ruslan Ismailov]

Sorry, I mean that in case: rowCount=10, aIndex= -2, than exeption is not
thrown, as I see. Or I wrong? Because exeption thrown now 

((PRUint32)aIndex > rowCount && aIndex != -1)

When (aIndex > rowCount or -aIndex > rowCount)  and (aIndex != -1).
correct would be ((PRUint32)aIndex > rowCount || aIndex < -1)

Or I wrong?

Comment 8

[Ruslan Ismailov]

Hmm. I wrong again :-). I incorrect calculate unsigned int. This exeption is not
throu if aIndex=-10 and rowCound=MaxInt-1, bit this is not possible cause.

Comment 9

[Ruslan Ismailov]

Attachment: Proposed patch v 1.1

Patch, implement comment #4 proposal.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111296 [details] [diff] [review]
Proposed patch v 1.1

Remove the extra parens about the two parts of the condition, and sr=bzbarsky.

Comment 11

[Ruslan Ismailov]

Sorry, you little misguide me. Should I remove this parens?

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in.