Bug 468176, r=enn, sr=neil

content/xul/document/src/nsXULDocument.cpp

@@ -1019,12 +1019,24 @@ nsXULDocument::AttributeChanged(nsIDocument* aDocument,
                     (bl->mAttribute == nsGkAtoms::_asterix)) {
                     nsCOMPtr<nsIDOMElement> listenerEl
                         = do_QueryReferent(bl->mListener);
-                    if (listenerEl) {
+                    nsCOMPtr<nsIContent> l = do_QueryInterface(listenerEl);
+                    if (l) {
+                      nsAutoString currentValue;
+                      PRBool hasAttr = l->GetAttr(kNameSpaceID_None,
+                                                  aAttribute,
+                                                  currentValue);
+                      // We need to update listener only if we're
+                      // (1) removing an existing attribute,
+                      // (2) adding a new attribute or
+                      // (3) changing the value of an attribute.
+                      PRBool needsAttrChange =
+                          attrSet != hasAttr || !value.Equals(currentValue);
                       nsDelayedBroadcastUpdate delayedUpdate(domele,
                                                              listenerEl,
                                                              aAttribute,
                                                              value,
-                                                             attrSet);
+                                                             attrSet,
+                                                             needsAttrChange);
                       mDelayedAttrChangeBroadcasts.AppendElement(delayedUpdate);
                     }
                 }
@@ -3283,17 +3295,18 @@ nsXULDocument::EndUpdate(nsUpdateType aUpdateType)
             mDelayedAttrChangeBroadcasts.SwapElements(
                                              delayedAttrChangeBroadcasts);
             for (PRUint32 i = 0; i < length; ++i) {
-                nsCOMPtr<nsIContent> listener =
-                    do_QueryInterface(delayedAttrChangeBroadcasts[i].mListener);
                 nsIAtom* attrName = delayedAttrChangeBroadcasts[i].mAttrName;
-                nsString value = delayedAttrChangeBroadcasts[i].mAttr;
-                if (delayedAttrChangeBroadcasts[i].mSetAttr) {
-                    listener->SetAttr(kNameSpaceID_None, attrName, value,
-                                      PR_TRUE);
-                }
-                else {
-                    listener->UnsetAttr(kNameSpaceID_None, attrName,
-                                        PR_TRUE);
+                if (delayedAttrChangeBroadcasts[i].mNeedsAttrChange) {
+                    nsCOMPtr<nsIContent> listener =
+                        do_QueryInterface(delayedAttrChangeBroadcasts[i].mListener);
+                    nsString value = delayedAttrChangeBroadcasts[i].mAttr;
+                    if (delayedAttrChangeBroadcasts[i].mSetAttr) {
+                        listener->SetAttr(kNameSpaceID_None, attrName, value,
+                                          PR_TRUE);
+                    } else {
+                        listener->UnsetAttr(kNameSpaceID_None, attrName,
+                                            PR_TRUE);
+                    }
                 }
                 nsCOMPtr<nsIContent> broadcaster =
                     do_QueryInterface(delayedAttrChangeBroadcasts[i].mBroadcaster);

content/xul/document/src/nsXULDocument.h

@@ -705,20 +705,22 @@ protected:
                                nsIDOMElement* aListener,
                                const nsAString &aAttr)
       : mBroadcaster(aBroadcaster), mListener(aListener), mAttr(aAttr),
-        mSetAttr(PR_FALSE) {}
+        mSetAttr(PR_FALSE), mNeedsAttrChange(PR_FALSE) {}
 
       nsDelayedBroadcastUpdate(nsIDOMElement* aBroadcaster,
                                nsIDOMElement* aListener,
                                nsIAtom* aAttrName,
                                const nsAString &aAttr,
-                               PRBool aSetAttr)
+                               PRBool aSetAttr,
+                               PRBool aNeedsAttrChange)
       : mBroadcaster(aBroadcaster), mListener(aListener), mAttr(aAttr),
-        mAttrName(aAttrName), mSetAttr(aSetAttr) {}
+        mAttrName(aAttrName), mSetAttr(aSetAttr),
+        mNeedsAttrChange(aNeedsAttrChange) {}
 
       nsDelayedBroadcastUpdate(const nsDelayedBroadcastUpdate& aOther)
       : mBroadcaster(aOther.mBroadcaster), mListener(aOther.mListener),
         mAttr(aOther.mAttr), mAttrName(aOther.mAttrName),
-        mSetAttr(aOther.mSetAttr) {}
+        mSetAttr(aOther.mSetAttr), mNeedsAttrChange(aOther.mNeedsAttrChange) {}
 
       nsCOMPtr<nsIDOMElement> mBroadcaster;
       nsCOMPtr<nsIDOMElement> mListener;
@@ -726,7 +728,8 @@ protected:
       // this is the value of the attribute.
       nsString                mAttr;
       nsCOMPtr<nsIAtom>       mAttrName;
-      PRBool                  mSetAttr;
+      PRPackedBool            mSetAttr;
+      PRPackedBool            mNeedsAttrChange;
     };
 
     nsTArray<nsDelayedBroadcastUpdate> mDelayedBroadcasters;

content/xul/document/test/Makefile.in

@@ -53,6 +53,7 @@ _TEST_FILES	= \
 		test_bug418216.xul \
 		test_bug445177.xul \
 		test_bug449457.xul \
+		test_bug468176.xul \
 		$(NULL)
 
 libs:: $(_TEST_FILES)

content/xul/document/test/test_bug468176.xul

@@ -0,0 +1,85 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
+<?xml-stylesheet href="/tests/SimpleTest/test.css" type="text/css"?>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=468176
+-->
+<window title="Test for Bug 468176"
+  id="test_bug468176_xul"
+  xmlns:html="http://www.w3.org/1999/xhtml"
+  xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+
+<body id="body" xmlns="http://www.w3.org/1999/xhtml">
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=468176">Mozilla Bug 468176</a>
+
+<xul:hbox id="b1" value="foo"/>
+
+<xul:hbox id="o1">
+  <xul:observes id="inner" element="b1" attribute="*"/>
+</xul:hbox>
+
+<pre id="test">
+  <script class="testbody" type="text/javascript">
+<![CDATA[
+  SimpleTest.waitForExplicitFinish();
+
+  var broadcastCount = 0;
+  function b_listener(evt) {
+    ++broadcastCount;
+  }
+
+  function do_test() {
+    var b1 = document.getElementById("b1");
+    var o1 = document.getElementById("o1");
+    var inner = document.getElementById("inner");
+    is(o1.getAttribute("value"), b1.getAttribute("value"), "Wrong value (1)");
+
+    inner.addEventListener("broadcast", b_listener, true);
+    b1.setAttribute("value", "bar");
+    is(o1.getAttribute("value"), b1.getAttribute("value"), "Wrong value (2)");
+    is(broadcastCount, 1, "Wrong value (3)");
+
+    b1.removeAttribute("value");
+    is(o1.hasAttribute("value"), b1.hasAttribute("value"), "Wrong value (4)");
+    is(broadcastCount, 2, "Wrong value (5)");
+
+    o1.setAttribute("value", "foo");
+    isnot(o1.getAttribute("value"), b1.getAttribute("value"), "Wrong value (6)");
+    is(broadcastCount, 2, "Wrong value (7)");
+
+    b1.setAttribute("value", "foobar");
+    is(o1.getAttribute("value"), b1.getAttribute("value"), "Wrong value (8)");
+    is(broadcastCount, 3, "Wrong value (9)");
+
+    b1.removeAttribute("value");
+    is(o1.getAttribute("value"), b1.getAttribute("value"), "Wrong value (10)");
+    is(broadcastCount, 4, "Wrong value (11)");
+
+    b1.removeAttribute("value");
+    is(o1.getAttribute("value"), b1.getAttribute("value"), "Wrong value (12)");
+    is(broadcastCount, 4, "Wrong value (13)");
+
+    o1.setAttribute("value", "bar");
+    b1.setAttribute("value", "bar"); // This should still dispatch 'broadcast'
+    is(o1.getAttribute("value"), b1.getAttribute("value"), "Wrong value (14)");
+    is(broadcastCount, 5, "Wrong value (15)");
+
+    //After removing listener, changes to broadcaster shouldn't have any effect.
+    o1.parentNode.removeChild(o1);
+    b1.setAttribute("value", "foo");
+    isnot(o1.getAttribute("value"), b1.getAttribute("value"), "Wrong value (16)");
+    is(broadcastCount, 5, "Wrong value (17)");
+
+    SimpleTest.finish();
+  }
+
+  addLoadEvent(do_test);
+]]>
+  </script>
+</pre>
+</body>
+</window>