Bug 18041 - DOMNodeRemoved events are sent twice
Summary: DOMNodeRemoved events are sent twice
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 6590
  Show dependency treegraph
 
Reported: 2008-03-24 00:51 PDT by Frank Illenberger
Modified: 2008-06-08 12:42 PDT (History)
2 users (show)

See Also:


Attachments
test case (297 bytes, text/html)
2008-03-27 14:58 PDT, Alexey Proskuryakov
no flags Details
Patch (2.33 KB, patch)
2008-05-15 13:16 PDT, Vincent Ricard
darin: review-
Details | Formatted Diff | Diff
Patch + expected file (3.71 KB, patch)
2008-05-25 13:49 PDT, Vincent Ricard
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frank Illenberger 2008-03-24 00:51:05 PDT
When a node is removed from a DOMDocument, observers of the DOMNodeRemoved event are notified twice for each change when they should only be notified once.

I could track down the reason for this behavior to ContainerNode.cpp where the same event is fired explicitly in line 390 and directly afterwards via willRemoveChild/dispatchChildRemovalEvents.

Most probably, the explicit sending of the event in line 390 has to be removed.
Comment 1 Alexey Proskuryakov 2008-03-27 14:58:11 PDT
Created attachment 20137 [details]
test case
Comment 2 Alexey Proskuryakov 2008-03-27 15:00:56 PDT
Confirmed with r31309.
Comment 3 Vincent Ricard 2008-05-15 13:16:00 PDT
Created attachment 21174 [details]
Patch

As mentioned in the report, remove the dispatchEvent line 390 fix the bug.
Comment 4 Darin Adler 2008-05-24 22:47:19 PDT
Comment on attachment 21174 [details]
Patch

Code change looks perfect. Oops!

The patch needs to include the results of the regression test. That's the DOMNodeRemovedEvent -expected.txt file.

The regression test should be turned into a text-only test so it doesn't dump the layout of the elements. That can be done by adding this code:

    if (window.layoutTestController)
        layoutTestController.dumpAsText();

to the test. By doing this, you'll make the test work as-is cross-platform and also eliminate the need to check in "pixel test results".

Please resubmit the patch with a corrected regression test, and we'll get this checked in.
Comment 5 Vincent Ricard 2008-05-25 13:49:23 PDT
Created attachment 21340 [details]
Patch + expected file

I rewrote the test case to use the shouldBe methods (and added the expected file).
Comment 6 Darin Adler 2008-05-25 13:50:30 PDT
Comment on attachment 21340 [details]
Patch + expected file

Looks fine. r=me
Comment 7 Darin Adler 2008-06-08 12:42:31 PDT
Committed revision 34450.