Bug 1205318 - make aria-owns loop alg more sophisticated, r=yzen
authorAlexander Surkov <surkov.alexander@gmail.com>
Tue, 29 Sep 2015 15:17:40 -0400
changeset 300251 f11627daf10e34e4109e39dbb2d652bda412cc95
parent 300250 8305fc7ffd1c2adc305f9047276dd2c1aed3c1c5
child 300252 31809ba78fed403021308e0db76d5b9c651e3589
push id1001
push userraliiev@mozilla.com
push dateMon, 18 Jan 2016 19:06:03 +0000
treeherdermozilla-release@8b89261f3ac4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersyzen
bugs1205318
milestone44.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1205318 - make aria-owns loop alg more sophisticated, r=yzen
accessible/generic/DocAccessible.cpp
accessible/generic/DocAccessible.h
accessible/tests/mochitest/common.js
accessible/tests/mochitest/tree/a11y.ini
accessible/tests/mochitest/tree/test_aria_owns.html
accessible/tests/mochitest/treeupdate/test_ariaowns.html
--- a/accessible/generic/DocAccessible.cpp
+++ b/accessible/generic/DocAccessible.cpp
@@ -1608,44 +1608,31 @@ DocAccessible::AddDependentIDsFor(Access
           // children invalidation (this happens immediately after the caching
           // is finished).
           nsIContent* dependentContent = iter.GetElem(id);
           if (dependentContent) {
             if (!HasAccessible(dependentContent)) {
               mInvalidationList.AppendElement(dependentContent);
             }
 
+            // Update ARIA owns cache.
             if (relAttr == nsGkAtoms::aria_owns) {
-              // Dependent content cannot point to other aria-owns content or
-              // their parents. Ignore it if so.
-              // XXX: note, this alg may make invalid the scenario when X owns Y
-              // and Y owns Z, we should have something smarter to handle that.
-              bool isvalid = true;
-              for (auto it = mARIAOwnsHash.Iter(); !it.Done(); it.Next()) {
-                Accessible* owner = it.Key();
-                nsIContent* parentEl = owner->GetContent();
-                while (parentEl && parentEl != dependentContent) {
-                  parentEl = parentEl->GetParent();
-                }
-                if (parentEl) {
-                  isvalid = false;
-                  break;
-                }
+              // ARIA owns cannot refer to itself or a parent. Ignore
+              // the element if so.
+              nsIContent* parentEl = relProviderEl;
+              while (parentEl && parentEl != dependentContent) {
+                parentEl = parentEl->GetParent();
               }
-              if (isvalid) {
-                // ARIA owns also cannot refer to itself or a parent.
-                nsIContent* parentEl = relProviderEl;
-                while (parentEl && parentEl != dependentContent) {
-                  parentEl = parentEl->GetParent();
-                }
-                if (parentEl) {
-                  isvalid = false;
-                }
 
-                if (isvalid) {
+              if (!parentEl) {
+                // ARIA owns element cannot refer to an element in parents chain
+                // of other ARIA owns element (including that ARIA owns element)
+                // if it's inside of a dependent element subtree of that
+                // ARIA owns element. Applied recursively.
+                if (!IsInARIAOwnsLoop(relProviderEl, dependentContent)) {
                   nsTArray<nsIContent*>* list =
                     mARIAOwnsHash.LookupOrAdd(aRelProvider);
                   list->AppendElement(dependentContent);
 
                   mARIAOwnsInvalidationList.AppendElement(
                     ARIAOwnsPair(aRelProvider, dependentContent));
                 }
               }
@@ -1755,16 +1742,55 @@ DocAccessible::RemoveDependentIDsFor(Acc
     // If the relation attribute is given then we don't have anything else to
     // check.
     if (aRelAttr)
       break;
   }
 }
 
 bool
+DocAccessible::IsInARIAOwnsLoop(nsIContent* aOwnerEl, nsIContent* aDependentEl)
+{
+  // ARIA owns element cannot refer to an element in parents chain of other ARIA
+  // owns element (including that ARIA owns element) if it's inside of
+  // a dependent element subtree of that ARIA owns element.
+  for (auto it = mARIAOwnsHash.Iter(); !it.Done(); it.Next()) {
+    Accessible* otherOwner = it.Key();
+    nsIContent* parentEl = otherOwner->GetContent();
+    while (parentEl && parentEl != aDependentEl) {
+      parentEl = parentEl->GetParent();
+    }
+
+    // The dependent element of this ARIA owns element contains some other ARIA
+    // owns element, make sure this ARIA owns element is not in a subtree of
+    // a dependent element of that other ARIA owns element. If not then
+    // continue a check recursively.
+    if (parentEl) {
+      nsTArray<nsIContent*>* childEls = it.UserData();
+      for (uint32_t idx = 0; idx < childEls->Length(); idx++) {
+        nsIContent* childEl = childEls->ElementAt(idx);
+        nsIContent* parentEl = aOwnerEl;
+        while (parentEl && parentEl != childEl) {
+          parentEl = parentEl->GetParent();
+        }
+        if (parentEl) {
+          return true;
+        }
+
+        if (IsInARIAOwnsLoop(aOwnerEl, childEl)) {
+          return true;
+        }
+      }
+    }
+  }
+
+  return false;
+}
+
+bool
 DocAccessible::UpdateAccessibleOnAttrChange(dom::Element* aElement,
                                             nsIAtom* aAttribute)
 {
   if (aAttribute == nsGkAtoms::role) {
     // It is common for js libraries to set the role on the body element after
     // the document has loaded. In this case we just update the role map entry.
     if (mContent == aElement) {
       SetRoleMapEntry(aria::GetRoleMap(aElement));
--- a/accessible/generic/DocAccessible.h
+++ b/accessible/generic/DocAccessible.h
@@ -432,16 +432,22 @@ protected:
    *
    * @param aRelProvider [in] accessible that element has relation attribute
    * @param aRelAttr     [in, optional] relation attribute
    */
   void RemoveDependentIDsFor(Accessible* aRelProvider,
                              nsIAtom* aRelAttr = nullptr);
 
   /**
+   * Return true if given ARIA owner element and its referred content make
+   * the loop closed.
+   */
+  bool IsInARIAOwnsLoop(nsIContent* aOwnerEl, nsIContent* aDependentEl);
+
+  /**
    * Update or recreate an accessible depending on a changed attribute.
    *
    * @param aElement   [in] the element the attribute was changed on
    * @param aAttribute [in] the changed attribute
    * @return            true if an action was taken on the attribute change
    */
   bool UpdateAccessibleOnAttrChange(mozilla::dom::Element* aElement,
                                     nsIAtom* aAttribute);
--- a/accessible/tests/mochitest/common.js
+++ b/accessible/tests/mochitest/common.js
@@ -449,21 +449,21 @@ function testAccessibleTree(aAccOrElmOrI
 
   // Test children.
   if ("children" in accTree && accTree["children"] instanceof Array) {
     var children = acc.children;
     var childCount = children.length;
 
     if (accTree.children.length != childCount) {
       for (var i = 0; i < Math.max(accTree.children.length, childCount); i++) {
-        var accChild;
+        var accChild = null, testChild = null;
         try {
+          testChild = accTree.children[i];
           accChild = children.queryElementAt(i, nsIAccessible);
 
-          testChild = accTree.children[i];
           if (!testChild) {
             ok(false, prettyName(acc) + " has an extra child at index " + i +
               " : " + prettyName(accChild));
             continue;
           }
 
           var key = Object.keys(testChild)[0];
           var roleName = "ROLE_" + key;
@@ -476,20 +476,20 @@ function testAccessibleTree(aAccOrElmOrI
 
           if (accChild.role !== testChild.role) {
             ok(false, prettyName(accTree) + " and " + prettyName(acc) +
               " have different children at index " + i + " : " +
               prettyName(testChild) + ", " + prettyName(accChild));
           }
           info("Matching " + prettyName(accTree) + " and " + prettyName(acc) +
                " child at index " + i + " : " + prettyName(accChild));
+
         } catch (e) {
-          ok(false, prettyName(accTree) + " has an extra child at index " + i +
+          ok(false, prettyName(accTree) + " is expected to have a child at index " + i +
              " : " + prettyName(testChild) + ", " + e);
-          throw e;
         }
       }
     } else {
       if (aFlags & kSkipTreeFullCheck) {
         for (var i = 0; i < childCount; i++) {
           var child = children.queryElementAt(i, nsIAccessible);
           testAccessibleTree(child, accTree.children[i], aFlags);
         }
--- a/accessible/tests/mochitest/tree/a11y.ini
+++ b/accessible/tests/mochitest/tree/a11y.ini
@@ -5,16 +5,17 @@ support-files =
 
 [test_applicationacc.xul]
 skip-if = true # Bug 561508
 [test_aria_globals.html]
 [test_aria_grid.html]
 [test_aria_imgmap.html]
 [test_aria_list.html]
 [test_aria_menu.html]
+[test_aria_owns.html]
 [test_aria_presentation.html]
 [test_aria_table.html]
 [test_brokencontext.html]
 [test_button.xul]
 [test_canvas.html]
 [test_combobox.xul]
 [test_cssoverflow.html]
 [test_dochierarchy.html]
new file mode 100644
--- /dev/null
+++ b/accessible/tests/mochitest/tree/test_aria_owns.html
@@ -0,0 +1,119 @@
+<!DOCTYPE html>
+<html>
+
+<head>
+  <title>@aria-owns attribute testing</title>
+
+  <link rel="stylesheet" type="text/css"
+        href="chrome://mochikit/content/tests/SimpleTest/test.css" />
+
+  <script type="application/javascript"
+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+
+  <script type="application/javascript"
+          src="../common.js"></script>
+  <script type="application/javascript"
+          src="../role.js"></script>
+
+  <script type="application/javascript">
+    ////////////////////////////////////////////////////////////////////////////
+    // Tests
+    ////////////////////////////////////////////////////////////////////////////
+
+    //enableLogging("tree"); // debug stuff
+
+    var gQueue = null;
+
+    function doTest()
+    {
+      var tree =
+        { SECTION: [ // t1_1
+          { SECTION: [ // t1_2
+            // no kids, no loop
+          ] }
+        ] };
+      testAccessibleTree("t1_1", tree);
+
+      tree =
+        { SECTION: [ // t2_1
+          { SECTION: [ // t2_2
+            { SECTION: [ // t2_3
+              // no kids, no loop
+            ] }
+          ] }
+        ] };
+      testAccessibleTree("t2_1", tree);
+
+      tree =
+        { SECTION: [ // t3_3
+          { SECTION: [ // t3_1
+            { SECTION: [ // t3_2
+              { SECTION: [ // DOM child of t3_2
+                // no kids, no loop
+              ] }
+            ] }
+          ] }
+        ] };
+      testAccessibleTree("t3_3", tree);
+
+      tree =
+        { SECTION: [ // t4_1
+          { SECTION: [ // DOM child of t4_1
+            // no kids, no loop
+          ] }
+        ] };
+      testAccessibleTree("t4_1", tree);
+
+      tree =
+        { SECTION: [ // t5_1
+          { SECTION: [ // DOM child of t5_1
+            { SECTION: [ // t5_2
+              { SECTION: [ // DOM child of t5_2
+                { SECTION: [ // t5_3
+                  { SECTION: [ // DOM child of t5_3
+                    // no kids, no loop
+                  ]}
+                ]}
+              ]}
+            ] }
+          ] }
+        ] };
+      testAccessibleTree("t5_1", tree);
+
+      SimpleTest.finish();
+    }
+
+    SimpleTest.waitForExplicitFinish();
+    addA11yLoadEvent(doTest);
+
+  </script>
+</head>
+
+<body>
+
+  <p id="display"></p>
+  <div id="content" style="display: none"></div>
+  <pre id="test">
+  </pre>
+
+  <div id="t1_1" aria-owns="t1_2"></div>
+  <div id="t1_2" aria-owns="t1_1"></div>
+
+  <div id="t2_2" aria-owns="t2_3"></div>
+  <div id="t2_1" aria-owns="t2_2"></div>
+  <div id="t2_3" aria-owns="t2_1"></div>
+
+  <div id="t3_1" aria-owns="t3_2"></div>
+  <div id="t3_2">
+    <div aria-owns="t3_3"></div>
+  </div>
+  <div id="t3_3" aria-owns="t3_1"></div>
+
+  <div id="t4_1"><div aria-owns="t4_1"></div></div>
+
+  <div id="t5_1"><div aria-owns="t5_2"></div>
+  <div id="t5_2"><div aria-owns="t5_3"></div></div>
+  <div id="t5_3"><div aria-owns="t5_1"></div></div>
+</body>
+
+</html>
--- a/accessible/tests/mochitest/treeupdate/test_ariaowns.html
+++ b/accessible/tests/mochitest/treeupdate/test_ariaowns.html
@@ -167,35 +167,23 @@
         return "Remove a container of ARIA ownded element";
       }
     }
 
     ////////////////////////////////////////////////////////////////////////////
     // Test
     ////////////////////////////////////////////////////////////////////////////
 
-    gA11yEventDumpToConsole = true;
-    enableLogging("tree"); // debug stuff
+    //gA11yEventDumpToConsole = true;
+    //enableLogging("tree"); // debug stuff
 
     var gQueue = null;
 
     function doTest()
     {
-      // nested and recursive aria-owns
-      var tree =
-        { SECTION: [ // container
-          { SECTION: [ // child
-            { SECTION: [ // mid div
-              { SECTION: [] } // grandchild
-            ] }
-          ] }
-        ] };
-      testAccessibleTree("container", tree);
-
-      // dynamic tests
       gQueue = new eventQueue();
 
       gQueue.push(new removeARIAOwns());
       gQueue.push(new setARIAOwns());
       gQueue.push(new appendEl());
       gQueue.push(new removeEl());
 
       gQueue.invoke(); // SimpleTest.finish() will be called in the end
@@ -209,22 +197,16 @@
 
 <body>
 
   <p id="display"></p>
   <div id="content" style="display: none"></div>
   <pre id="test">
   </pre>
 
-  <div id="container" aria-owns="child" aria-label="container"></div>
-  <div id="child" aria-label="child">
-    <div aria-owns="grandchild" aria-label="midchild"></div>
-  </div>
-  <div id="grandchild" aria-owns="container" aria-label="grandchild"></div>
-
   <div id="container2" aria-owns="t2_checkbox t2_button">
     <div role="button" id="t2_button"></div>
     <div role="checkbox" id="t2_checkbox">
       <span id="t2_span">
         <div id="t2_subdiv"></div>
       </span>
     </div>
   </div>