Bug 18041

Summary:

DOMNodeRemoved events are sent twice

Product:

WebKit

Reporter:

Frank Illenberger <illenberger>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, aroben

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac (Intel)

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

6590

Attachments:

Description	Flags
test case	none
Patch	darin: review-
Patch + expected file	darin: review+

Frank Illenberger

Reported 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.

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2008-03-27 14:58:11 PDT

Created attachment 20137 [details] test case

Alexey Proskuryakov

Comment 2 2008-03-27 15:00:56 PDT

Confirmed with r31309.

Vincent Ricard

Comment 3 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.

Darin Adler

Comment 4 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.

Vincent Ricard

Comment 5 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).

Darin Adler

Comment 6 2008-05-25 13:50:30 PDT

Comment on attachment 21340 [details] Patch + expected file Looks fine. r=me

Darin Adler

Comment 7 2008-06-08 12:42:31 PDT

Committed revision 34450.

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