Closed Bug 296230 Opened 19 years ago Closed 19 years ago

javascript removeNode only affects the view, NOT the DOM

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: nicolas, Assigned: peterv)

References

Details

Attachments

(3 files)

test case
(deleted), text/html
Details
Reduced Testcase
(deleted), text/html
Details
v1
(deleted), patch
jst
: review+
jst
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050601 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050601 Firefox/1.0+

I'm using javascript to populate an html table using the DOM functions
createElement, appendChild, etc. But there is a problem when I try to remove the
rows:
  I first loop in the childNodes of the tbody and remove some of the rows. The
result is immediately visible in the page.
  Then I reloop in the childNodes of the tbody to remove some other rows.
Surprisingly, the rows I removed first are still there (from a javascript point
of view) and if I try to remove one of those ghosts, I get the following error:

Error: uncaught exception: [Exception... "Node was not found"  code: "8"
nsresult: "0x80530008 (NS_ERROR_DOM_NOT_FOUND_ERR)"  location:
"http://localhost/functions.js Line: 58"]

If I go to the DOMInspector, the rows don't appear in the DOM tree, but are
still shown in the childNodes of the tbody in the Javascript Object view.

Here is the tree :
table
  tbody
    tr
      td
        input type=checkbox (checked: the row is to be removed)
      td
        text and image
    tr (same as above)
    tr (same as above)
    tr (same as above)

Reproducible: Always

Steps to Reproduce:
1. create a new file and paste this code:
<html>
<head>
<script type="text/javascript">
function removeFromList(tBody){
  n_tbody = document.getElementById(tBody);
  n_trs = n_tbody.childNodes;
  n_toRemove = Array();
  for(i_tr in n_trs){
    /* skip the non-tr nodes */
    if(n_trs[i_tr].nodeName==undefined ||
       n_trs[i_tr].nodeName.toLowerCase()!="tr") continue;
    /* get the checkbox directly (quick and dirty, without checks) */
    n_input = n_trs[i_tr].firstChild.firstChild;
    if(n_input.checked==true){
      n_toRemove.push(n_trs[i_tr]);
    }
  }
  /* remove the nodes */
  while(n_tr = n_toRemove.pop()) n_tbody.removeChild(n_tr); /* <-- error here */
}
</script>
</head>
<body>
<form>
<table>
<tbody id="foo">
<tr><td><input type="checkbox"/></td><td>cell1</td></tr>
<tr><td><input type="checkbox"/></td><td>cell 1</td></tr>
<tr><td><input type="checkbox"/></td><td>cell 2</td></tr>
<tr><td><input type="checkbox"/></td><td>cell 3</td></tr>
<tr><td><input type="checkbox"/></td><td>cell 4</td></tr>
</tbody>
</table>
<input type="button" value="remove" onclick="removeFromList('foo')"/>
</form>
</body>
</html>
2. Open the page with firefox, select AT LEAST TWO checkboxes and click "remove"
3. Select another checkbox and click "remove"

Actual Results:  
The first time, the rows are removed
The second time, I get a javascript error
--> the DOM view shows 3 lines, the Javascript object shows 4!

BUT: when only one checkbox is selected at a time, everything goes well.

Expected Results:  
The rows should be removed, the Javascript object and dom tree sould be synchronized
Attached file test case (deleted) — 
This is the proposed code that causes the error as a testcase. I do get an
error: 

Error: uncaught exception: [Exception... "Node was not found"  code: "8"
nsresult: "0x80530008 (NS_ERROR_DOM_NOT_FOUND_ERR)"  location:
"file:///C:/Documents%20and%20Settings/Adam%20Guthrie/Desktop/testcase.html
Line: 19"]

but I'm not sure whether this has to do with just bad JavaScript of Firefox's
implementation of it.
I've checked and double-checked the javascript code, but maybe I missed something.
But the thing that really strikes me is that the Javascript Object (childNodes
of the tbody object) does not match the DOM tree.
Attached file Reduced Testcase (deleted) — 
The initial code does not do what the author intended since it uses the in
operator with an array and thus loops through both the numeric array properties
and the length and item properties.

It does seem to reveal something strange though, see the testcase, try running
it with the for loop commented out to see the difference in behaviour.

->Core: DOM Core
Assignee: nobody → general
Component: General → DOM: Core
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → Trunk
Depends on: 100499
Attached patch v1 (deleted) — — Splinter Review 
Assignee: general → peterv
Status: UNCONFIRMED → ASSIGNED

        Attachment #196017 -
        Flags: superreview?(jst)

        Attachment #196017 -
        Flags: review?(jst)

    
    

    
  
Comment on attachment 196017 [details] [diff] [review]
v1

Ugh, attached wrong patch, pretend |*vp = nsnull| is actually |*vp =
JSVAL_NULL|

    
    

    
  
Comment on attachment 196017 [details] [diff] [review]
v1

Looks good, but I'd say we should probably use JSVAL_VOID here, not JSVAL_NULL.
If you today do alert(nodelist[500000]), you'll get undefined, not null (except
if you're perverted enough to have that many nodes in the list, or course), so
accessing an entry by an index that used to exist but doesn't exist any more,
you should get back the same value as you did when the entry never did exist.

r+sr=jst

        Attachment #196017 -
        Flags: superreview?(jst)

        Attachment #196017 -
        Flags: superreview+

        Attachment #196017 -
        Flags: review?(jst)

        Attachment #196017 -
        Flags: review+

    
  
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
No longer depends on: 100499
Resolution: --- → FIXED
Depends on: 310068
Depends on: 310927
Depends on: 332238

    
  
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: