223349 - [FIXr]DOMSubtreeModified not fired in XUL, SVG, or XML

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

I've gotten sick of the code duplication invoved in all the child list
mutations.  So I'm hoisting these functions up to nsGenericElement (oops, XUL
still loses out) to make sure that people don't fix just HTML from now on.

Patch coming up that fixes this bug (XUL gets fixed by copying some code to make
it look like nsGenericElement... sigh... perhaps we need nsReallyGenericElement?
 ;) )

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 133911 [details] [diff] [review]
Proposed patch

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

> +  virtual PRBool InternalAppendChildTo(nsIContent* aKid) {
> +    return mChildren.AppendElement(aKid);;

Remove the second ;

> Index: content/base/src/nsGenericElement.cpp
> ===================================================================
> RCS file: /cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v
> retrieving revision 3.296
> diff -p -u -1 -3 -r3.296 nsGenericElement.cpp
> --- content/base/src/nsGenericElement.cpp	22 Oct 2003 06:09:29 -0000	3.296
> +++ content/base/src/nsGenericElement.cpp	23 Oct 2003 06:08:10 -0000
> @@ -2470,26 +2470,182 @@ nsGenericElement::GetListenerManager(nsI
>  
>    *aResult = entry->mListenerManager;
>    NS_ADDREF(*aResult);
>  
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
>  nsGenericElement::DoneCreatingElement()
>  {
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsGenericElement::InsertChildAt(nsIContent* aKid,
> +                                PRUint32 aIndex,
> +                                PRBool aNotify,
> +                                PRBool aDeepSetDocument)
> +{
> +  NS_PRECONDITION(nsnull != aKid, "null ptr");

NS_PRECONDITION(aKid, ...)

> +  mozAutoDocUpdate updateBatch(mDocument, UPDATE_CONTENT_MODEL, aNotify);
> +  
> +  PRBool rv = InternalInsertChildAt(aKid, aIndex);
> +  if (rv) {

No need for rv. I'd just return NS_OK on (!InternalInsertChildAt) but up to
you.

> +    NS_ADDREF(aKid);
> +    aKid->SetParent(this);
> +    nsRange::OwnerChildInserted(this, aIndex);
> +    if (mDocument) {
> +      aKid->SetDocument(mDocument, aDeepSetDocument, PR_TRUE);
> +      if (aNotify) {
> +        mDocument->ContentInserted(this, aKid, aIndex);
> +      }
> +
> +      if (nsGenericElement::HasMutationListeners(this, NS_EVENT_BITS_MUTATION_NODEINSERTED)) {

No need for nsGenericElement::

> +        nsCOMPtr<nsIDOMEventTarget> node(do_QueryInterface(aKid));
> +        nsMutationEvent mutation;
> +        mutation.eventStructType = NS_MUTATION_EVENT;
> +        mutation.message = NS_MUTATION_NODEINSERTED;
> +        mutation.mTarget = node;

mutation.mTarget = do_QueryInterface(aKid); and drop node.

> +
> +        nsCOMPtr<nsIDOMNode> relNode(do_QueryInterface(NS_STATIC_CAST(nsIContent *, this)));
> +        mutation.mRelatedNode = relNode;

mutation.mRelatedNode = do_QueryInterface(this); and drop relNode.

> +
> +        nsEventStatus status = nsEventStatus_eIgnore;
> +        aKid->HandleDOMEvent(nsnull, &mutation, nsnull, NS_EVENT_FLAG_INIT, &status);
> +      }
> +    }
> +  }
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsGenericElement::ReplaceChildAt(nsIContent* aKid,
> +                                 PRUint32 aIndex,
> +                                 PRBool aNotify,
> +                                 PRBool aDeepSetDocument)
> +{
> +  NS_PRECONDITION(nsnull != aKid, "null ptr");

NS_PRECONDITION(aKid, ...)

> +  nsIContent* oldKid = GetChildAt(aIndex);
> +  mozAutoDocUpdate updateBatch(mDocument, UPDATE_CONTENT_MODEL, aNotify);
> +
> +  nsRange::OwnerChildReplaced(this, aIndex, oldKid);
> +  PRBool rv = InternalReplaceChildAt(aKid, aIndex);
> +  if (rv) {

No need for rv. I'd just return NS_OK on (!InternalReplaceChildAt) but up to
you.

> +    NS_ADDREF(aKid);
> +    aKid->SetParent(this);
> +    if (mDocument) {
> +      aKid->SetDocument(mDocument, aDeepSetDocument, PR_TRUE);
> +      if (aNotify) {
> +        mDocument->ContentReplaced(this, oldKid, aKid, aIndex);
> +      }
> +      if (nsGenericElement::HasMutationListeners(this, NS_EVENT_BITS_MUTATION_SUBTREEMODIFIED)) {

No need for nsGenericElement::

> +        nsMutationEvent mutation;
> +        mutation.eventStructType = NS_MUTATION_EVENT;
> +        mutation.message = NS_MUTATION_SUBTREEMODIFIED;
> +        mutation.mTarget = do_QueryInterface(this);
> +        mutation.mRelatedNode = do_QueryInterface(oldKid);
> +    
> +        nsEventStatus status = nsEventStatus_eIgnore;
> +        HandleDOMEvent(nsnull, &mutation, nsnull,
> +                       NS_EVENT_FLAG_INIT, &status);
> +      }
> +    }
> +
> +    if (oldKid) {
> +      oldKid->SetDocument(nsnull, PR_TRUE, PR_TRUE);
> +      oldKid->SetParent(nsnull);
> +      NS_RELEASE(oldKid);
> +    }
> +  }
> +  
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsGenericElement::AppendChildTo(nsIContent* aKid, PRBool aNotify,
> +                                PRBool aDeepSetDocument)
> +{
> +  NS_PRECONDITION(nsnull != aKid && this != aKid, "null ptr");

NS_PRECONDITION(aKid && this != aKid, ...)

> +  mozAutoDocUpdate updateBatch(mDocument, UPDATE_CONTENT_MODEL, aNotify);
> +  
> +  PRBool rv = InternalAppendChildTo(aKid);
> +  if (rv) {

No need for rv. I'd just return NS_OK on (!InternalAppendChildTo) but up to
you.

> +    NS_ADDREF(aKid);
> +    aKid->SetParent(this);
> +    // ranges don't need adjustment since new child is at end of list
> +    if (mDocument) {
> +      aKid->SetDocument(mDocument, aDeepSetDocument, PR_TRUE);
> +      if (aNotify) {
> +        mDocument->ContentAppended(this, GetChildCount() - 1);
> +      }
> +
> +      if (nsGenericElement::HasMutationListeners(this, NS_EVENT_BITS_MUTATION_NODEINSERTED)) {

No need for nsGenericElement::

> +        nsCOMPtr<nsIDOMEventTarget> node(do_QueryInterface(aKid));
> +        nsMutationEvent mutation;
> +        mutation.eventStructType = NS_MUTATION_EVENT;
> +        mutation.message = NS_MUTATION_NODEINSERTED;
> +        mutation.mTarget = node;

mutation.mTarget = do_QueryInterface(aKid); and drop node.

> +
> +        nsCOMPtr<nsIDOMNode> relNode(do_QueryInterface(NS_STATIC_CAST(nsIContent *, this)));
> +        mutation.mRelatedNode = relNode;

mutation.mRelatedNode = do_QueryInterface(this); and drop relNode.

> +
> +        nsEventStatus status = nsEventStatus_eIgnore;
> +        aKid->HandleDOMEvent(nsnull, &mutation, nsnull, NS_EVENT_FLAG_INIT, &status);
> +      }
> +    }
> +  }
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsGenericElement::RemoveChildAt(PRUint32 aIndex, PRBool aNotify)
> +{
> +  mozAutoDocUpdate updateBatch(mDocument, UPDATE_CONTENT_MODEL, aNotify);
> +  
> +  nsIContent* oldKid = GetChildAt(aIndex);
> +  if (oldKid ) {
> +

Remove the blank line? And shouldn't the mozAutoDocUpdate be inside this if?

> +    if (nsGenericElement::HasMutationListeners(this, NS_EVENT_BITS_MUTATION_NODEREMOVED)) {

No need for nsGenericElement::

> +      nsCOMPtr<nsIDOMEventTarget> node(do_QueryInterface(oldKid));
> +      nsMutationEvent mutation;
> +      mutation.eventStructType = NS_MUTATION_EVENT;
> +      mutation.message = NS_MUTATION_NODEREMOVED;
> +      mutation.mTarget = node;

mutation.mTarget = do_QueryInterface(oldKid); and drop node.

> +
> +      nsCOMPtr<nsIDOMNode> relNode(do_QueryInterface(NS_STATIC_CAST(nsIContent *, this)));
> +      mutation.mRelatedNode = relNode;

mutation.mRelatedNode = do_QueryInterface(this); and drop relNode.

> +
> +      nsEventStatus status = nsEventStatus_eIgnore;
> +      oldKid->HandleDOMEvent(nsnull, &mutation, nsnull,
> +                             NS_EVENT_FLAG_INIT, &status);
> +    }
> +
> +    nsRange::OwnerChildRemoved(this, aIndex, oldKid);
> +
> +    PRBool rv = InternalRemoveChildAt(aIndex);
> +    if (rv && aNotify && mDocument) {

No need for rv.

> Index: content/html/content/src/nsGenericHTMLElement.h
> ===================================================================

> +  virtual PRBool InternalAppendChildTo(nsIContent* aKid) {
> +    return mChildren.AppendElement(aKid);;

Remove the second ;

> Index: content/xul/content/src/nsXULElement.cpp
> ===================================================================

> @@ -1983,39 +1983,52 @@ nsXULElement::ReplaceChildAt(nsIContent*

> +            if (HasMutationListeners(this,
> +                                     NS_EVENT_BITS_MUTATION_SUBTREEMODIFIED)) {
> +                nsMutationEvent mutation;
> +                mutation.eventStructType = NS_MUTATION_EVENT;
> +                mutation.message = NS_MUTATION_SUBTREEMODIFIED;
> +                mutation.mTarget = do_QueryInterface(NS_STATIC_CAST(nsIStyledContent*, this));

Do you really need to cast here?

> @@ -2026,64 +2039,67 @@ nsXULElement::AppendChildTo(nsIContent* 

> +            if (HasMutationListeners(this,
> +                                     NS_EVENT_BITS_MUTATION_NODEINSERTED)) {
> +                nsCOMPtr<nsIDOMEventTarget> node(do_QueryInterface(aKid));
> +                nsMutationEvent mutation;
> +                mutation.eventStructType = NS_MUTATION_EVENT;
> +                mutation.message = NS_MUTATION_NODEINSERTED;
> +                mutation.mTarget = node;

mutation.mTarget = do_QueryInterface(aKid); and drop node.

>  
> -          nsCOMPtr<nsIDOMNode> relNode(do_QueryInterface(NS_STATIC_CAST(nsIStyledContent*,this)));
> -          mutation.mRelatedNode = relNode;
> +                nsCOMPtr<nsIDOMNode> relNode(do_QueryInterface(NS_STATIC_CAST(nsIStyledContent*,this)));
> +                mutation.mRelatedNode = relNode;

mutation.mRelatedNode = do_QueryInterface(this); and drop relNode.


> Index: content/svg/content/src/nsSVGElement.h
> ===================================================================

> +  virtual PRBool InternalAppendChildTo(nsIContent* aKid) {
> +    return mChildren.AppendElement(aKid);;

Remove the second ;

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to comments

Yes, nsXULElement needs the casts due to ambiguous inheritance from
nsISupports.

Fixed the rest.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 133973 [details] [diff] [review]
Patch updated to comments

Sweet!

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

> +  /**
> +   * InsertChildAt/ReplaceChildAt/AppendChildTo/RemoveChildAt subclass
> +   * hooks.  These methods are called to perform the actual moving
> +   * around of content nodes in child lists.  The return value should
> +   * be true if something changed, false otherwise.
> +   *
> +   * These methods should not change the refcound on the kids in question;

refcount :-)

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

> +NS_IMETHODIMP
> +nsGenericElement::RemoveChildAt(PRUint32 aIndex, PRBool aNotify)
> +{
> +  nsIContent* oldKid = GetChildAt(aIndex);
> +  if (oldKid ) {

Remove the extra space before )

> Index: content/xul/content/src/nsXULElement.cpp
> ===================================================================

> @@ -1936,41 +1936,40 @@ nsXULElement::InsertChildAt(nsIContent* 

> +      mutation.mRelatedNode =
> +          do_QueryInterface(NS_STATIC_CAST(nsIStyledContent*,this));

Space after the comma.

>  nsXULElement::AppendChildTo(nsIContent* aKid, PRBool aNotify, PRBool aDeepSetDocument)

> +                mutation.mRelatedNode =
> +                    do_QueryInterface(NS_STATIC_CAST(nsIStyledContent*,this));

Space after the comma.

>  nsXULElement::RemoveChildAt(PRUint32 aIndex, PRBool aNotify)

> +      mutation.mRelatedNode =
> +          do_QueryInterface(NS_STATIC_CAST(nsIStyledContent*,this));

Space after the comma.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Address those comments too

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 134205 [details] [diff] [review]
Address those comments too

I don't really like that the addref is done outside the |Internal| functions.
However i guess it would be risky to do the release there so I can't really
think of a better option. (And these methods should hopefully go away soon
anyhow).

r=sicking

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Checked in.