Bug 35590 - DOM insertion mutation events should dispatch after a node is attached to the render tree
Summary: DOM insertion mutation events should dispatch after a node is attached to the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-03-02 11:06 PST by Antoine Quint
Modified: 2010-03-04 10:17 PST (History)
3 users (show)

See Also:


Attachments
Testcase showing the width value of computedStyle for an appended element after various mutation events are dispatched (1.13 KB, text/html)
2010-03-02 11:07 PST, Antoine Quint
no flags Details
Patch (5.70 KB, patch)
2010-03-03 07:54 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (6.96 KB, patch)
2010-03-03 08:45 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (8.27 KB, patch)
2010-03-04 09:26 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2010-03-02 11:06:01 PST
The insertion mutation events, such as DOMNodeInserted and DOMNodeInsertedIntoDocument, are dispatched before ->attach() is called on the inserted node. This causes computed style to be meaningless in handlers for such events and differs from the Firefox and Opera implementations that provide such useful information.

It's worth noting that DOMSubtreeModified is dispatched _after_ the element was attached to the render tree.
Comment 1 Antoine Quint 2010-03-02 11:07:07 PST
Created attachment 49819 [details]
Testcase showing the width value of computedStyle for an appended element after various mutation events are dispatched
Comment 2 Antoine Quint 2010-03-02 11:26:29 PST
rdar://problem/7707492
Comment 3 Antoine Quint 2010-03-03 07:54:18 PST
Created attachment 49910 [details]
Patch
Comment 4 Adam Roben (:aroben) 2010-03-03 07:57:56 PST
Comment on attachment 49910 [details]
Patch

The code changes look good. But I think you should test insertBefore and replaceChild, too, since you modified those.
Comment 5 Antoine Quint 2010-03-03 08:45:23 PST
Created attachment 49912 [details]
Patch
Comment 6 Darin Adler 2010-03-03 08:58:13 PST
Comment on attachment 49912 [details]
Patch

Moving the childrenChanged call after the attach call might lead to bugs like the one fixed in <http://trac.webkit.org/changeset/55462> because all the code run inside attach will be done while functions like HTMLSelectElement::childrenChanged have not yet been called. For example, the select element's list of items will still be wrong. And Element::childrenChanged won't run, which has something to do with updating style sharing data structures.

The test cases check only the mutation event aspect of this change, so if we had not moved the childrenChanged function calls, the test would still pass. The test doesn't show us possible problems created or bugs fixed by moving the childrenChanged calls.
Comment 7 Antoine Quint 2010-03-04 02:16:56 PST
I've moved only the dispatchChildInsertionEvents() calls after the render tree attachments, and left the childrenChanged() calls where they were which still fixes the original issue, but breaks a fair few layout tests, specifically for app cache.

By the looks of it dispatchChildInsertionEvents() actually does a bit more than dispatching just the insertion DOM events and some of its calls probably alter the process here. My guess is that dispatchChildInsertionEvents() should split off the actual event dispatching code to be on its own so that part only can be moved post render tree attachment.
Comment 8 Antoine Quint 2010-03-04 09:26:56 PST
Created attachment 50029 [details]
Patch
Comment 9 Antoine Quint 2010-03-04 09:29:43 PST
So I split off the internal-to-WebCore notification bits from
dispatchChildInsertionEvents() into a new static function
notifyChildInserted(), and now dispatchChildInsertionEvents() is called _after_
attachment of the node to the render tree.
Comment 10 WebKit Commit Bot 2010-03-04 10:17:31 PST
Comment on attachment 50029 [details]
Patch

Clearing flags on attachment: 50029

Committed r55532: <http://trac.webkit.org/changeset/55532>
Comment 11 WebKit Commit Bot 2010-03-04 10:17:36 PST
All reviewed patches have been landed.  Closing bug.