Closed Bug 288392 Opened 15 years ago Closed 13 years ago

DOMSubtreeModified event does not fire at all

Categories

(Core :: DOM: Events, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: andrew.halliday, Assigned: smaug)

References

()

Details

Attachments

(5 files, 7 obsolete files)

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.
Attached file A test case for this bug. (obsolete) —
Only tests DOMNodeInserted, DOMAttrModified & DOMNodeRemoved so far.
> 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.
Whiteboard: DUEPME
Whiteboard: DUEPME → DUPEME
(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?!
> 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...
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.
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.
This testcase actually works when DOMSubtreeModified is dispatched.
Though, more testcases needed.
Attachment #179152 - Attachment is obsolete: true
Attached patch WIP (obsolete) — Splinter Review
Something like this might be possible, but batching makes this a bit
difficult for content authors to understand, I think.
Attached patch WIP2 (obsolete) — Splinter Review
a bit better.
Attachment #252311 - Attachment is obsolete: true
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.
Attachment #252312 - Attachment is obsolete: true
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 :(
Attachment #252366 - Flags: review?(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 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.
Attachment #252366 - Flags: review?(bzbarsky) → review?(peterv)
Comment on attachment 252366 [details] [diff] [review]
maybe even a possible patch :)

I'll update the patch to handle .innerHTML, .textContent, normalize() ...
Attachment #252366 - Flags: review?(peterv)
Added cases for innerHTML, textContent and normalize
Attachment #252592 - Attachment is obsolete: true
Attached patch up-to-date (obsolete) — Splinter Review
Patch updated to take account also bug 367781.
Assignee: events → Olli.Pettay
Attachment #252683 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #253604 - Flags: review?(peterv)
Attachment #252683 - Flags: review?(peterv)
Attached patch up-to-date (obsolete) — Splinter Review
Attachment #253604 - Attachment is obsolete: true
Attachment #254993 - Flags: review?(peterv)
Attachment #253604 - Flags: review?(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? :-)
(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.
Attached patch better commentsSplinter Review
Attachment #254993 - Attachment is obsolete: true
Attachment #257983 - Flags: review?(peterv)
Attachment #254993 - Flags: review?(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.
Attachment #257983 - Flags: review?(peterv) → review+
Attachment #257983 - Flags: superreview?(jst)
Attachment #257983 - Flags: superreview?(jst) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Attached patch mochitestsSplinter Review
Attachment #259522 - Flags: review?(jst)
Whiteboard: DUPEME
Couldn't we put the depth member on nsIDocument and make the method that checks it non-virtual?
Sure. I'll make a patch.
Attachment #259522 - Flags: review?(jst)
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.