Bug 35590

Summary: DOM insertion mutation events should dispatch after a node is attached to the render tree
Product: WebKit Reporter: Antoine Quint <ml>
Component: DOMAssignee: Antoine Quint <ml>
Severity: Normal CC: ap, aroben, commit-queue
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
Testcase showing the width value of computedStyle for an appended element after various mutation events are dispatched
Patch none

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
Comment 3 Antoine Quint 2010-03-03 07:54:18 PST
Created attachment 49910 [details]
Comment 4 Adam Roben (:aroben) 2010-03-03 07:57:56 PST
Comment on attachment 49910 [details]

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]
Comment 6 Darin Adler 2010-03-03 08:58:13 PST
Comment on attachment 49912 [details]

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]
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]

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.