Bug 443081 – ARIA name from child content is incorrectly including child elements that have display:none, r=aaronlev, MarcoZ

accessible/src/base/nsAccessible.cpp

@@ -1593,15 +1593,6 @@ nsresult nsAccessible::AppendFlatStringFromContentNode(nsIContent *aContent, nsA
   nsAutoString textEquivalent;
   if (!aContent->IsNodeOfType(nsINode::eHTML)) {
     if (aContent->IsNodeOfType(nsINode::eXUL)) {
-      nsCOMPtr<nsIPresShell> shell = GetPresShell();
-      if (!shell) {
-        return NS_ERROR_FAILURE;  
-      }
-      nsIFrame *frame = shell->GetPrimaryFrameFor(aContent);
-      if (!frame || !frame->GetStyleVisibility()->IsVisible()) {
-        return NS_OK;
-      }
-
       nsCOMPtr<nsIDOMXULLabeledControlElement> labeledEl(do_QueryInterface(aContent));
       if (labeledEl) {
         labeledEl->GetLabel(textEquivalent);
@@ -1661,8 +1652,17 @@ nsresult nsAccessible::AppendFlatStringFromSubtree(nsIContent *aContent, nsAStri
   if (isAlreadyHere) {
     return NS_OK;
   }
+
   isAlreadyHere = PR_TRUE;
-  nsresult rv = AppendFlatStringFromSubtreeRecurse(aContent, aFlatString);
+
+  nsCOMPtr<nsIPresShell> shell = GetPresShell();
+  NS_ENSURE_TRUE(shell, NS_ERROR_FAILURE);
+
+  nsIFrame *frame = shell->GetPrimaryFrameFor(aContent);
+  PRBool isHidden = (!frame || !frame->GetStyleVisibility()->IsVisible());
+  nsresult rv = AppendFlatStringFromSubtreeRecurse(aContent, aFlatString,
+                                                   isHidden);
+
   isAlreadyHere = PR_FALSE;
 
   if (NS_SUCCEEDED(rv) && !aFlatString->IsEmpty()) {
@@ -1681,7 +1681,10 @@ nsresult nsAccessible::AppendFlatStringFromSubtree(nsIContent *aContent, nsAStri
   return rv;
 }
 
-nsresult nsAccessible::AppendFlatStringFromSubtreeRecurse(nsIContent *aContent, nsAString *aFlatString)
+nsresult
+nsAccessible::AppendFlatStringFromSubtreeRecurse(nsIContent *aContent,
+                                                 nsAString *aFlatString,
+                                                 PRBool aIsRootHidden)
 {
   // Depth first search for all text nodes that are decendants of content node.
   // Append all the text into one flat string
@@ -1698,10 +1701,25 @@ nsresult nsAccessible::AppendFlatStringFromSubtreeRecurse(nsIContent *aContent,
   }
 
   // There are relevant children: use them to get the text.
+  nsCOMPtr<nsIPresShell> shell = GetPresShell();
+  NS_ENSURE_TRUE(shell, NS_ERROR_FAILURE);
+
   PRUint32 index;
   for (index = 0; index < numChildren; index++) {
-    AppendFlatStringFromSubtreeRecurse(aContent->GetChildAt(index), aFlatString);
+    nsCOMPtr<nsIContent> childContent = aContent->GetChildAt(index);
+
+    // Walk into hidden subtree if the the root parent is also hidden. This
+    // happens when the author explictly uses a hidden label or description.
+    if (!aIsRootHidden) {
+      nsIFrame *childFrame = shell->GetPrimaryFrameFor(childContent);
+      if (!childFrame || !childFrame->GetStyleVisibility()->IsVisible())
+        continue;
+    }
+
+    AppendFlatStringFromSubtreeRecurse(childContent, aFlatString,
+                                       aIsRootHidden);
   }
+
   return NS_OK;
 }
 

accessible/src/base/nsAccessible.h

@@ -197,7 +197,22 @@ protected:
   // helper method to verify frames
   static nsresult GetFullKeyName(const nsAString& aModifierName, const nsAString& aKeyName, nsAString& aStringOut);
   static nsresult GetTranslatedString(const nsAString& aKey, nsAString& aStringOut);
-  nsresult AppendFlatStringFromSubtreeRecurse(nsIContent *aContent, nsAString *aFlatString);
+
+  /**
+   * Walk into subtree and calculate the string which is used as the accessible
+   * name or description.
+   *
+   * @param aContent      [in] traversed content
+   * @param aFlatString   [in, out] result string
+   * @param aIsRootHidden [in] specifies whether root content (we started to
+   *                      traverse from) is hidden, in this case the result
+   *                      string is calculated from hidden children
+   *                      (this is used when hidden root content is explicitly
+   *                      specified as label or description by author)
+   */
+  nsresult AppendFlatStringFromSubtreeRecurse(nsIContent *aContent,
+                                              nsAString *aFlatString,
+                                              PRBool aIsRootHidden);
 
   // Helpers for dealing with children
   virtual void CacheChildren();

accessible/tests/mochitest/test_nsIAccessible_name.html

@@ -58,7 +58,15 @@
       
       // Gets the name from text nodes contained by nested elements
       testName("btn_labelledby_mixed", "nomore text");
-  
+
+      // Gets the name from text nodes contained by nested elements, ignores
+      // hidden elements (bug 443081).
+      testName("btn_labelledby_mixed_hidden_child", "nomore text2");
+
+      // Gets the name from hidden text nodes contained by nested elements, 
+      // (label element is hidden entirely), (bug 443081).
+      testName("btn_labelledby_mixed_hidden", "lala more hidden text");
+
       // Gets the name from text nodes contained by nested elements having block
       // representation (every text node value in the name should be devided by
       // spaces)
@@ -113,6 +121,10 @@
       // children.
       testName("btn_children", "14");
 
+      // ARIA role option is presented allowing the name calculation from
+      // visible children (bug 443081).
+      testName("lb_opt1_children_hidden", "i am visible");
+
 
       //////////////////////////////////////////////////////////////////////////
       // title attribute
@@ -160,6 +172,25 @@
           aria-labelledby="labelledby_mixed">3</button>
   <br/>
 
+  <!-- the name from subtree, mixed/hidden content -->
+  <span id="labelledby_mixed_hidden_child">
+    no<span>more 
+      <span style="display: none;">hidden</span>
+      text2
+      <span style="visibility: hidden">hidden2</span>
+    </span>
+  </span>
+  <button id="btn_labelledby_mixed_hidden_child"
+          aria-labelledby="labelledby_mixed_hidden_child">3.1</button>
+  <br/>
+
+  <!-- the name from subtree, mixed/completely hidden content -->
+  <span id="labelledby_mixed_hidden"
+        style="display: none;">lala <span>more hidden </span>text</span></span>
+  <button id="btn_labelledby_mixed_hidden"
+          aria-labelledby="labelledby_mixed_hidden">3.2</button>
+  <br/>
+
   <!-- the name from subtree, mixed content, block structure -->
   <div id="labelledby_mixed_block"><div>text</div>more text</div></div>
   <button id="btn_labelledby_mixed_block"
@@ -228,6 +259,14 @@
   <!-- name from children -->
   <span id="btn_children" role="button">14</span>
 
+  <!-- name from children, hidden children -->
+  <div role="listbox" tabindex="0">
+    <div id="lb_opt1_children_hidden" role="option" tabindex="0">
+      <span>i am visible</span>
+      <span style="display:none">i am hidden</span>
+    </div>
+  </div>
+
   <!-- name from title attribute -->
   <span id="btn_title" role="group" title="title">15</span>
 </body>

accessible/tests/mochitest/test_nsIAccessible_name.xul

@@ -72,6 +72,14 @@
       testName("btn_labelledby_mixed_menulist",
                "nomore text selected item more text");
       
+      // Gets the name from text nodes contained by nested elements, ignores
+      // hidden elements (bug 443081).
+      testName("btn_labelledby_mixed_hidden_child", "nomore text2");
+
+      // Gets the name from hidden text nodes contained by nested elements, 
+      // (label element is hidden entirely), (bug 443081)
+      testName("btn_labelledby_mixed_hidden", "lala more hidden text");
+
 
       //////////////////////////////////////////////////////////////////////////
       // Name for nsIDOMXULLabeledControlElement.
@@ -139,6 +147,7 @@
       // tooltiptext (if nothing above isn't presented then tooltiptext is used)
       testName("box_tooltiptext", "tooltiptext label");
 
+
       //////////////////////////////////////////////////////////////////////////
       // Name from the @title attribute of <toolbaritem/> (original bug 237249).
 
@@ -163,6 +172,10 @@
       // children.
       testName("box_children", "14");
 
+      // ARIA role option is presented allowing the name calculation from
+      // the visible children (bug 443081)
+      testName("lb_opt1_children_hidden", "i am visible");
+
 
       SimpleTest.finish();
     }
@@ -203,6 +216,18 @@
   <button id="btn_labelledby_mixed"
           aria-labelledby="labelledby_mixed"/>
 
+  <!-- the name from subtree, mixed/hidden content -->
+  <description id="labelledby_mixed_hidden_child">no<description>more <description hidden="true">hidden</description>text2</description></description>
+  <button id="btn_labelledby_mixed_hidden_child"
+          aria-labelledby="labelledby_mixed_hidden_child"/>
+
+  <!-- the name from subtree, mixed/completely hidden content -->
+  <description id="labelledby_mixed_hidden"
+               hidden="true">lala <description>more hidden </description>text</description>
+  <button id="btn_labelledby_mixed_hidden"
+          aria-labelledby="labelledby_mixed_hidden"/>
+  <br/>
+
   <!-- the name from subtree, mixed content, ignore items of menulist -->
   <description id="labelledby_mixed_menulist">
     no<description>more text</description>
@@ -281,5 +306,13 @@
   <!-- name from children -->
   <box id="box_children" role="button">14</box>
 
+  <!-- name from children, hidden children -->
+  <vbox role="listbox" tabindex="0">
+    <hbox id="lb_opt1_children_hidden" role="option" tabindex="0">
+      <description>i am visible</description>
+      <description style="display:none">i am hidden</description>
+    </hbox>
+  </vbox>
+
 </window>