288392 - DOMSubtreeModified event does not fire at all

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Andrew]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; .NET CLR 1.0.3705; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

In no situation I have tried (many) does the DOMSubtreeModified event fire.

According to page 27 of the DOM2 Events Module:

"This is a general event for notification of all changes to the document. It 
can be used instead of the more specific events listed below. 
[ ... <snip> ...]
The target of this event is the lowest common parent of the changes which have 
taken place. This event is dispatched after any other events caused by the 
mutation have fired."

For me, this means, that EVERY modification of the subtree, including node 
replacement, removing/adding nodes, changing attributes ... should fire it. As 
said in the spec, it also should be fired together with one of the 
events "listed below":
 - DOMNodeInserted
 - DOMNodeRemoved
 - DOMNodeRemovedFromDocument
 - DOMNodeInsertedInDocument
 - DOMAttrModified
 - DOMCharacterDataModified

Once this works, the entire Mutations (1.6.4) section will work in Mozilla, 
possibly passing the entirety of section 1.6 (Event Modules)?

Reproducible: Always

Steps to Reproduce:
1.Attach an event listener to any node in a document.
2.Change any aspect of any node under the node being 'listened to'.
Actual Results:  
1.Attach an event listener to any node in a document.
2.Change any aspect of any node under the node being 'listened to'. For 
example, change an attribute value.
3.DOMAttrModified fires.

Thats it ...

Expected Results:  
1.Attach an event listener to any node in a document.
2.Change an attribute value.
3.DOMAttrModified fires.
4.DOMSubtreeModified fires.

Comment 1

[Andrew]

Attachment: A test case for this bug.

Only tests DOMNodeInserted, DOMAttrModified & DOMNodeRemoved so far.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> For me, this means, that EVERY modification of the subtree,

The UA is allowed to batch in an arbitrary way, per spec.  So we could fire say
every 3000 years and be perfectly compliant.  Not useful, but there it is... 
There is actually no way to write a test that in finite time tells apart
compliant from non-compliant behavior on this portion of DOM events.

Comment 3

[Andrew]

(In reply to comment #2)
> The UA is allowed to batch in an arbitrary way, per spec.  So we could fire 
say
> every 3000 years and be perfectly compliant.  Not useful, but there it is... 
> There is actually no way to write a test that in finite time tells apart
> compliant from non-compliant behavior on this portion of DOM events.

Haha okay okay, technically correct but that is the biggest copout!  What ever 
happened to practicality?  Whats with trying to avoid implementing this?  Is 
it really that much of a pain?!

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Is it really that much of a pain?!

Basically, yes.  Worse yet, it's a pain to use.  The entire section of the DOM
spec is ill-defined, more or less.  Consider event handlers that rearrange the
DOM while the DOMAttrModified event is propagating.  Where should the
DOMSubtreeModified event be fired?  And when?  Should there be one
DOMSubtreeModified per mutation?  Or should we actually batch (which is what
consumers seem to want)?  If so how?

If someone submits a patch we'll probably take it, if it's reasonable...

Comment 5

[Nikolas 'Atrus' Coukouma]

DOMSubtreeModified works between 1.0rc1 and 1.0.6 (inclusive), but not in Deer
Park Alpha 1-2. This is true of the nightly builds from the branches. It does
not work in 0.9.3 and earlier.

The oldest nightly build I've found that works is 2004-10-29-06-0.11/mac .
2004-10-29-07-trunk/mac fails. The mac part is just the platform I'm testing on.
I haven't tried others and I doubt it matters.

Comment 6

[clc4tts]

I can confirm that this is a problem for Firefox 2.0.0.1 on Windows XP; DOMSubtreeModified does not fire for me either. Please fix this.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a bit modified testcase.

This testcase actually works when DOMSubtreeModified is dispatched.
Though, more testcases needed.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP

Something like this might be possible, but batching makes this a bit
difficult for content authors to understand, I think.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP2

a bit better.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 252312 [details] [diff] [review]
WIP2


>+void
>+nsDocument::MutationEventDispatched(nsINode* aTarget)
>+{
>+  --mSubtreeModifiedDepth;
>+  if (!mSubtreeModifiedDepth) {
>+    nsCOMPtr<nsPIDOMWindow> window;
>+    window = do_QueryInterface(GetScriptGlobalObject());
>+    if (window && !window->HasMutationListeners(NS_EVENT_BITS_MUTATION_SUBTREEMODIFIED)) {
>+      return;
>+    }

Note to myself; should have
mSubtreeModifiedTargets.Clear();
before return.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: maybe even a possible patch :)

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase for DOMSubtreeModifed when the root of the subtree is not in document

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 252366 [details] [diff] [review]
maybe even a possible patch :)

Bz, maybe you want to say something about this ;)

With the patch DOMSubtreeModified is dispatched according to
"This event is dispatched after any other events caused by the mutation(s) have occurred."

DOMSubtreeModified gets fired only once if a mutation event causes new mutations.

DOMSubtreeModified is *SO* underspecified/ill-defined,/whatever :(

Comment 14

[Boris Zbarsky [:bzbarsky]]

I won't be able to really look at this in the near future (like weeks).  :(  Already too swamped with reviews.

I do think that it's desirable to only fire DOMSubtreeModified only once for things like normalize(), range operations, innerHTML sets, however that's accomplished...

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 252366 [details] [diff] [review]
maybe even a possible patch :)

Then maybe peterv has more time.

I'll write some more testcases for innerHTML etc.
We can easily prevent possible extra DOMSubtreeModified events using mozAutoSubtreeModified.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 252366 [details] [diff] [review]
maybe even a possible patch :)

I'll update the patch to handle .innerHTML, .textContent, normalize() ...

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase for DOMSubtreeModifed when the root of the subtree is not in document

Added cases for innerHTML, textContent and normalize

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: batches DOMSubtreeModifed events in .innerHTML, .textContent etc.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up-to-date

Patch updated to take account also bug 367781.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up-to-date

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 254993 [details] [diff] [review]
up-to-date

>Index: content/base/public/nsIDocument.h
>===================================================================

>@@ -822,16 +822,29 @@ public:

>+   * To make this easy and painless, use the mozAutoSubtreeModified helper class.

Do we want to enforce that by making this private and mozAutoSubtreeModified a friend?

>+class mozAutoSubtreeModified

>+  void UpdateTarget(nsIDocument* aSubtreeOwner, nsINode* aTarget) {

Brace on it's own line.

>Index: content/base/src/nsDocument.cpp
>===================================================================

>+  NS_ASSERTION(mSubtreeModifiedDepth != 0 || mSubtreeModifiedTargets.Count() == 0,

Long line.

>+               "mSubtreeModifiedTargets not cleared after dispatching?");
>+  ++mSubtreeModifiedDepth;
>+  if (aTarget && mSubtreeModifiedTargets.IndexOf(aTarget) == -1) {

Or you could let MutationEventDispatched deal with the duplicates (I think it already handles them).

>Index: content/events/src/nsEventListenerManager.cpp
>===================================================================

>+      window->SetMutationListeners((aType == NS_MUTATION_SUBTREEMODIFIED)
>+                                   ? kAllMutationBits
>+                                   : MutationBitForEventType(aType));

? and : should go at the end of the line, not the beginning.

I'm a bit at a loss how you determine the second parameter to the mozAutoSubtreeModified constructor. Sometimes it's the container, sometimes it's null, want to enlighten me? :-)

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #21)
> I'm a bit at a loss how you determine the second parameter to the
> mozAutoSubtreeModified constructor. Sometimes it's the container, sometimes
> it's null, want to enlighten me? :-)
> 
When it is null, mozAutoSubtreeModified just batches mutations, so
that dispatching possible DOMSubtreeModified is delayed until 
destructing the outermost mozAutoSubtreeModified.
The null target is useful for example in SetInnerHTML, where
we don't want to dispatch any events if nothing is modified
(<div id="foo"></div>  document.getElementById('foo').innerHTML = "";)
or we want to dispatch only one mutation after all the mutations
.innerHTML does have been done.

Updated patch with better comments coming.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better comments

Comment 24

[Peter Van der Beken [:peterv]]

Comment on attachment 257983 [details] [diff] [review]
better comments

>Index: content/base/public/nsIDocument.h
>===================================================================

>+ * mozAutoSubtreeModified batches DOM mutations so that possible
>+ * DOMSubtreeModified event is dispatched when the outermost
>+ * mozAutoSubtreeModified object is deleted.

... so that a DOMSubtreeModified event is dispatched, if necessary, when ...?

>+   *                      Can be nsnull, in which case mozAutoSubtreeModified
>+   *                      is used just to batch DOM mutations.

just used

>Index: content/base/src/nsDocument.h
>===================================================================

>+  virtual PRBool MutationEventBeingDispatched()

Not sure about the name, MutationEventWillBeDispatched wouldn't be correct either?
I don't think this needs to be virtual.

Sorry about the delay.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Attachment: mochitests

Comment 26

[Boris Zbarsky [:bzbarsky]]

Couldn't we put the depth member on nsIDocument and make the method that checks it non-virtual?

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Sure. I'll make a patch.