Bug 3295 – cellIndex is always zero

• Status: VERIFIED
• Resolution: FIXED
• Component: DOM

[Jonathan Hurshman]

The cellIndex property of TD nodes is always 0, regardless of the actual position of the cell in the row.

[Jonathan Hurshman]

Created attachment 2128 [details]
A testcase for this bug

[Jonathan Hurshman]

(This bug was copied from Apple's Bug Reporter #3756860)

[Allan Sandfeld Jensen]

Fixed in Konqueror 

[Maks Orlovich]

FYI, http://bugs.kde.org/show_bug.cgi?id=58799#c7  and 
http://websvn.kde.org/*checkout*/trunk/tests/khtmltests/regression/tests/unsorted/58799.html?rev=426346 . 
 
Previously it wasn't quite right.  
 

[Justin Garcia]

Created attachment 2448 [details]
Proposed patch

[Dave Hyatt]

+    const NodeImpl * node = this;
+    int index = 0;
+    do {
+        node = node->previousSibling();
+        if(node && (node->id() == ID_TD || node->id() == ID_TH))
+            index++;
+    } while(node);

for (const NodeImpl* node = node->previousSibling(); node; node = node->previousSibling())
    if (node->id() == ID_TH || node->id() == ID_TH)
        index++;

... reads better to me then a do construct that still ends up null-checking.

[Dave Hyatt]

+    const NodeImpl * node = this;
+    int index = 0;
+    do {
+        node = node->previousSibling();
+        if(node && (node->id() == ID_TD || node->id() == ID_TH))
+            index++;
+    } while(node);

for (const NodeImpl* node = node->previousSibling(); node; node = node->previousSibling()) {
    if (node->id() == ID_TD || node->id() == ID_TH)
        index++;
}

... reads better to me then a do construct that still ends up null-checking.

[Dave Hyatt]

node = previousSibling().

Yes, I can write code. Really. ;)

[Dave Hyatt]

Comment on attachment 2448 [details]
Proposed patch

r=me, would prefer for loop, but don't really feel strongly. :)

[Justin Garcia]

Created attachment 2449 [details]
Proposed patch with better coding style :-)

[Dave Hyatt]

Comment on attachment 2449 [details]
Proposed patch with better coding style :-)

r=me, fix the 8-space indentation to be 4-space though.

[Harri Porten]

Created attachment 2456 [details]
Remove outdated commnet

I don't think the comment is valid anymore.

[Harri Porten]

Created attachment 2457 [details]
Remove outdated comment

I don't think the comment is valid anymore.

[Darin Adler]

Patch looks good, and since Justin has commit privileges I'm not going to land it for him. Justin, you 
should commit this!

[Darin Adler]

The for loop idiom is pretty nice, nicer than the original do/while. There's also a good while idiom for this 
sort of thing that you can see in functions like HTMLOptionElementImpl::text.

[Anders Carlsson]

*** Bug 3623 has been marked as a duplicate of this bug. ***

[Joost de Valk (AlthA)]

Fixed in 10.4.2