Bug 496783 - setParent shouldn't be virtual, r=marcoz, davidb

--HG--
rename : accessible/tests/mochitest/test_accessnode_invalidation.html => accessible/tests/mochitest/test_invalidate_accessnode.html

accessible/src/base/nsAccessible.h

@@ -203,9 +203,8 @@ public:
 
   /**
    * Set accessible parent.
-   * XXX: shouldn't be virtual, bug 496783
    */
-  virtual void SetParent(nsIAccessible *aParent);
+  void SetParent(nsIAccessible *aParent);
 
   /**
    * Set first accessible child.

accessible/src/html/nsHTMLTextAccessible.cpp

@@ -232,9 +232,10 @@ nsresult
 nsHTMLLIAccessible::Shutdown()
 {
   if (mBulletAccessible) {
-    // Ensure that weak pointer to this is nulled out
+    // Ensure that pointer to this is nulled out.
     mBulletAccessible->Shutdown();
   }
+
   nsresult rv = nsLinkableAccessible::Shutdown();
   mBulletAccessible = nsnull;
   return rv;
@@ -283,7 +284,7 @@ void nsHTMLLIAccessible::CacheChildren()
 
   if (mBulletAccessible) {
     mBulletAccessible->SetNextSibling(mFirstChild);
-    mBulletAccessible->SetParent(this); // Set weak parent;
+    mBulletAccessible->SetParent(this);
     SetFirstChild(mBulletAccessible);
     ++ mAccChildCount;
   }
@@ -294,8 +295,7 @@ void nsHTMLLIAccessible::CacheChildren()
 nsHTMLListBulletAccessible::
   nsHTMLListBulletAccessible(nsIDOMNode* aDomNode, nsIWeakReference* aShell,
                              const nsAString& aBulletText) :
-    nsLeafAccessible(aDomNode, aShell), mWeakParent(nsnull),
-    mBulletText(aBulletText)
+    nsLeafAccessible(aDomNode, aShell), mBulletText(aBulletText)
 {
   mBulletText += ' '; // Otherwise bullets are jammed up against list text
 }
@@ -312,8 +312,6 @@ nsresult
 nsHTMLListBulletAccessible::Shutdown()
 {
   mBulletText.Truncate();
-  mWeakParent = nsnull;
-
   return nsLeafAccessible::Shutdown();
 }
 
@@ -343,17 +341,12 @@ nsHTMLListBulletAccessible::GetStateInternal(PRUint32 *aState, PRUint32 *aExtraS
   return NS_OK;
 }
 
-void
-nsHTMLListBulletAccessible::SetParent(nsIAccessible *aParent)
-{
-  mParent = nsnull;
-  mWeakParent = aParent;
-}
-
 NS_IMETHODIMP
-nsHTMLListBulletAccessible::GetParent(nsIAccessible **aParentAccessible)
+nsHTMLListBulletAccessible::GetParent(nsIAccessible **aParent)
 {
-  NS_IF_ADDREF(*aParentAccessible = mWeakParent);
+  NS_ENSURE_ARG_POINTER(aParent);
+
+  NS_IF_ADDREF(*aParent = mParent);
   return NS_OK;
 }
 

accessible/src/html/nsHTMLTextAccessible.h

@@ -109,11 +109,7 @@ public:
 
   // nsIAccessible
   NS_IMETHOD GetName(nsAString& aName);
-
-  // Don't cache via unique ID -- bullet accessible shares the same dom node as
-  // this LI accessible. Also, don't cache via mParent/SetParent(), prevent
-  // circular reference since li holds onto us.
-  NS_IMETHOD GetParent(nsIAccessible **aParentAccessible);
+  NS_IMETHOD GetParent(nsIAccessible **aParent);
 
   // nsAccessNode
   virtual nsresult Shutdown();
@@ -121,7 +117,6 @@ public:
   // nsAccessible
   virtual nsresult GetRoleInternal(PRUint32 *aRole);
   virtual nsresult GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState);
-  virtual void SetParent(nsIAccessible *aParent);
   virtual nsresult AppendTextTo(nsAString& aText, PRUint32 aStartOffset,
                                 PRUint32 aLength);
 
@@ -132,7 +127,6 @@ protected:
   // nsIAnonymousFrame::GetText() ? However, in practice storing the bullet text
   // here should not be a problem if we invalidate the right parts of
   // the accessibility cache when mutation events occur.
-  nsIAccessible *mWeakParent;
   nsString mBulletText;
 };
 

accessible/tests/mochitest/Makefile.in

@@ -68,7 +68,6 @@ _TEST_FILES =\
 		states.js \
 		table.js \
 		value.js \
-		test_accessnode_invalidation.html \
 		test_actions.xul \
 		test_actions_anchors.html \
 		test_actions_aria.html \
@@ -103,6 +102,8 @@ _TEST_FILES =\
 		test_events_valuechange.html \
 		test_groupattrs.xul \
 		test_groupattrs.html \
+		test_invalidate_accessnode.html \
+		test_invalidate_elmli.html \
 		test_name.html \
 		test_name.xul \
 		test_name_button.html \

accessible/tests/mochitest/events.js

@@ -34,17 +34,26 @@ function waitForEvent(aEventType, aTarget, aFunc, aContext, aArg1, aArg2)
 {
   var handler = {
     handleEvent: function handleEvent(aEvent) {
-      if (!aTarget || aTarget == aEvent.DOMNode) {
-        unregisterA11yEventListener(aEventType, this);
 
-        window.setTimeout(
-          function ()
-          {
-            aFunc.call(aContext, aArg1, aArg2);
-          },
-          0
-        );
+      if (aTarget) {
+        if (aTarget instanceof nsIAccessible &&
+            aTarget != aEvent.accessible)
+          return;
+
+        if (aTarget instanceof nsIDOMNode &&
+            aTarget != aEvent.DOMNode)
+          return;
       }
+
+      unregisterA11yEventListener(aEventType, this);
+
+      window.setTimeout(
+        function ()
+        {
+          aFunc.call(aContext, aArg1, aArg2);
+        },
+        0
+      );
     }
   };
 
@@ -91,6 +100,12 @@ function unregisterA11yEventListener(aEventType, aEventHandler)
  */
 const INVOKER_ACTION_FAILED = 1;
 
+/**
+ * Return value of eventQueue.onFinish. Indicates eventQueue should not finish
+ * tests.
+ */
+const DO_NOT_FINISH_TEST = 1;
+
 /**
  * Common invoker checker (see eventSeq of eventQueue).
  */
@@ -145,8 +160,8 @@ function invokerChecker(aEventType, aTarget)
  *     getID: function(){} // returns invoker ID
  *   };
  *
- * @param  aEventType     [optional] the default event type (isn't used if
- *                        invoker defines eventSeq property).
+ * @param  aEventType  [in, optional] the default event type (isn't used if
+ *                      invoker defines eventSeq property).
  */
 function eventQueue(aEventType)
 {
@@ -240,8 +255,10 @@ function eventQueue(aEventType)
       gA11yEventApplicantsCount--;
       listenA11yEvents(false);
 
-      this.onFinish();
-      SimpleTest.finish();
+      var res = this.onFinish();
+      if (res != DO_NOT_FINISH_TEST)
+        SimpleTest.finish();
+
       return;
     }
 
@@ -521,6 +538,54 @@ function eventQueue(aEventType)
   this.mEventSeqIdx = -1;
 }
 
+
+/**
+ * Deal with action sequence. Used when you need to execute couple of actions
+ * each after other one.
+ */
+function sequence()
+{
+  /**
+   * Append new sequence item.
+   *
+   * @param  aProcessor  [in] object implementing interface
+   *                      {
+   *                        // execute item action
+   *                        process: function() {},
+   *                        // callback, is called when item was processed
+   *                        onProcessed: function() {}
+   *                      };
+   * @param  aEventType  [in] event type of expected event on item action
+   * @param  aTarget     [in] event target of expected event on item action
+   * @param  aItemID     [in] identifier of item
+   */
+  this.append = function sequence_append(aProcessor, aEventType, aTarget,
+                                         aItemID)
+  {
+    var item = new sequenceItem(aProcessor, aEventType, aTarget, aItemID);
+    this.items.push(item);
+  }
+
+  /**
+   * Process next sequence item.
+   */
+  this.processNext = function sequence_processNext()
+  {
+    this.idx++;
+    if (this.idx >= this.items.length) {
+      ok(false, "End of sequence: nothing to process!");
+      SimpleTest.finish();
+      return;
+    }
+
+    this.items[this.idx].startProcess();
+  }
+
+  this.items = new Array();
+  this.idx = -1;
+}
+
+
 ////////////////////////////////////////////////////////////////////////////////
 // Private implementation details.
 ////////////////////////////////////////////////////////////////////////////////
@@ -637,3 +702,42 @@ function dumpInfoToDOM(aInfo, aDumpNode)
   container.textContent = aInfo;
   dumpElm.appendChild(container);
 }
+
+
+////////////////////////////////////////////////////////////////////////////////
+// Sequence
+
+/**
+ * Base class of sequence item.
+ */
+function sequenceItem(aProcessor, aEventType, aTarget, aItemID)
+{
+  // private
+  
+  this.startProcess = function sequenceItem_startProcess()
+  {
+    this.queue.invoke();
+  }
+  
+  var item = this;
+  
+  this.queue = new eventQueue();
+  this.queue.onFinish = function()
+  {
+    aProcessor.onProcessed();
+    return DO_NOT_FINISH_TEST;
+  }
+  
+  var invoker = {
+    invoke: function invoker_invoke() {
+      return aProcessor.process();
+    },
+    getID: function invoker_getID()
+    {
+      return aItemID;
+    },
+    eventSeq: [ new invokerChecker(aEventType, aTarget) ]
+  };
+  
+  this.queue.push(invoker);
+}

accessible/tests/mochitest/role.js

@@ -38,6 +38,7 @@ const ROLE_ROW = nsIAccessibleRole.ROLE_ROW;
 const ROLE_ROWHEADER = nsIAccessibleRole.ROLE_ROWHEADER;
 const ROLE_SECTION = nsIAccessibleRole.ROLE_SECTION;
 const ROLE_SLIDER = nsIAccessibleRole.ROLE_SLIDER;
+const ROLE_STATICTEXT = nsIAccessibleRole.ROLE_STATICTEXT;
 const ROLE_TABLE = nsIAccessibleRole.ROLE_TABLE;
 const ROLE_TEXT_CONTAINER = nsIAccessibleRole.ROLE_TEXT_CONTAINER;
 const ROLE_TEXT_LEAF = nsIAccessibleRole.ROLE_TEXT_LEAF;

accessible/tests/mochitest/test_invalidate_elmli.html

@@ -0,0 +1,168 @@
+<!DOCTYPE html>
+<html>
+
+<head>
+  <title>Test HTML li and listitem bullet accessible cache</title>
+  <link rel="stylesheet" type="text/css"
+        href="chrome://mochikit/content/tests/SimpleTest/test.css" />
+
+  <script type="application/javascript"
+          src="chrome://mochikit/content/MochiKit/packed.js"></script>
+  <script type="application/javascript"
+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+
+  <script type="application/javascript"
+          src="chrome://mochikit/content/a11y/accessible/common.js"></script>
+  <script type="application/javascript"
+          src="chrome://mochikit/content/a11y/accessible/role.js"></script>
+  <script type="application/javascript"
+          src="chrome://mochikit/content/a11y/accessible/events.js"></script>
+
+  <script type="application/javascript">
+
+    ////////////////////////////////////////////////////////////////////////////
+    // Helpers
+
+    function testDefunctAccessible(aAcc, aNodeOrId)
+    {
+      if (aNodeOrId)
+        ok(!isAccessible(aNodeOrId),
+           "Accessible for " + aNodeOrId + " wasn't properly shut down!");
+
+      ok(!aAcc.firstChild, "There is first child for shut down accessible!");
+      ok(!aAcc.lastChild, "There is last child for shut down accessible!");
+      ok(!aAcc.children.length, "There are children for shut down accessible!");
+
+      // nextSibling
+      var success = false;
+      try {
+        aAcc.nextSibling;
+      } catch (e) {
+        success = (e.result == Components.results.NS_ERROR_FAILURE);
+      }
+
+      ok(success, "There is next sibling for shut down accessible!");
+
+      // previousSibling
+      var success = false;
+      try {
+        aAcc.previousSibling;
+      } catch (e) {
+        success = (e.result == Components.results.NS_ERROR_FAILURE);
+      }
+
+      ok(success, "There is previous sibling for shut down accessible!");
+
+      // parent
+      var success = false;
+      try {
+        aAcc.parent;
+      } catch (e) {
+        success = (e.result == Components.results.NS_ERROR_FAILURE);
+      }
+
+      ok(success, "There is parent for shut down accessible!");
+    }
+
+    function testLiAccessibleTree()
+    {
+      // Test accessible tree.
+      var accTree = {
+        role: ROLE_LISTITEM,
+        children: [
+          {
+            role: ROLE_STATICTEXT,
+            children: []
+          },
+          {
+            role: ROLE_TEXT_LEAF,
+            children: []
+          }
+        ]
+      };
+
+      testAccessibleTree("li", accTree);
+    }
+
+    ////////////////////////////////////////////////////////////////////////////
+    // Sequence item processors
+
+    function hideProcessor()
+    {
+      this.liNode = getNode("li");
+      this.li = getAccessible(this.liNode);
+      this.bullet = this.li.firstChild;
+
+      this.process = function hideProcessor_process()
+      {
+        this.liNode.style.display = "none";
+      }
+
+      this.onProcessed = function hideProcessor_onProcessed()
+      {
+        window.setTimeout(
+          function(aArg1, aArg2)
+          {
+            testDefunctAccessible(aArg1, aArg2);
+            gSequence.processNext();
+          },
+          0, this.li, this.liNode
+        );
+      }
+    };
+
+    function showProcessor()
+    {
+      this.liNode = getNode("li");
+
+      this.process = function showProcessor_process()
+      {
+        this.liNode.style.display = "list-item";
+      }
+
+      this.onProcessed = function showProcessor_onProcessed()
+      {
+        testLiAccessibleTree();
+        SimpleTest.finish();
+      }
+    };
+
+    ////////////////////////////////////////////////////////////////////////////
+    // Test
+
+    var gSequence = null;
+
+    function doTest()
+    {
+      testLiAccessibleTree();
+
+      gSequence = new sequence();
+
+      gSequence.append(new hideProcessor(), EVENT_HIDE, getAccessible("li"),
+                       "hide HTML li");
+      gSequence.append(new showProcessor(), EVENT_SHOW, getNode("li"),
+                       "show HTML li");
+
+      gSequence.processNext(); // SimpleTest.finish() will be called in the end
+    }
+
+    SimpleTest.waitForExplicitFinish();
+    addA11yLoadEvent(doTest);
+  </script>
+</head>
+<body>
+
+  <a target="_blank"
+     title="setParent shouldn't be virtual"
+     href="https://bugzilla.mozilla.org/show_bug.cgi?id=496783">Mozilla Bug 496783</a>
+
+  <p id="display"></p>
+  <div id="content" style="display: none"></div>
+  <pre id="test">
+  </pre>
+
+  <ul>
+    <li id="li">item1</li>
+  </ul>
+</body>
+</html>