Closed Bug 443081 Opened 12 years ago Closed 12 years ago

ARIA name from child content is incorrectly including child elements that have display:none

Categories

(Core :: Disability Access APIs, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: annam, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30; .NET CLR 3.0.04506.648)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

I orginally noticed this issue with an element of role "option", but this issue conceivably happens for other roles / situations where name is being computed from child content.

In this case, the accName is currently being generated by concatenating the text content of *all* child elements, whereas elements that have display:none should be excluded from name computation.

Reproducible: Always

Steps to Reproduce:
1. Use html similar to follows

<div id="lb" role="listbox" tabindex="0">
  <div id="opt1" role="option" tabindex="0">
    <span>i am visible</span>
    <span style="display:none">i am hidden</span>
  </div>
</div>

2. When the focus moves to option element "opt1" watch for accName.
Actual Results:  
accName shows as "i am visible i am hidden"

Expected Results:  
accName to show as "i am visible"
Blocks: aria, 191a11y
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
Version: unspecified → Trunk
Thanks Sri. I like the suggestion in your email to use hidden elements only when they are explicitly pointed to put aria-labelledby.
Keywords: access
Surkov & Marco, I don't even have a working environment anymore, but I'd like to fix this for Sri/Google, this they were nice enough to report it.

Can you try this replacement for the loop in nsAccessible::AppendFlatStringFromSubtreeRecurse() ?

  // There are relevant children: use them to get the text.
  PRUint32 index;
  nsCOMPtr<nsIPresShell> shell = GetPresShell();
  NS_ENSURE_TRUE(shell, NS_ERROR_FAILURE);
  for (index = 0; index < numChildren; index++) {
    nsIContent *childContent = aContent->GetChildAt(index);
    NS_ENSURE_TRUE(childContent, NS_ERROR_FAILURE);
    nsIFrame *frame = shell->GetPrimaryFrameFor(childContent);
    if (!frame || !frame->GetStyleVisibility()->IsVisible()) {
      // The child frame is visible, we may want to ignore this
      // part of the subtree
      nsIFrame *parentFrame = shell->GetPrimaryFrameFor(aContent);
      if (parentFrame && parentFrame->GetStyleVisibility()->IsVisible()) {
        // The parent frame is not also hidden, which means we hadn't already
        // decided to walk into this subtree and use the contents.
        continue; // Ignore: try next child
      }
      else {  // This item is hidden but we will use it anyway
        // We have already decided to walk into this subtree (the parent is hidden)
        // This happens when the author explictly uses a hidden label or description
        /* Fall through and get content */
      }
    }
    AppendFlatStringFromSubtreeRecurse(childContent, aFlatString);
  }
  return NS_OK;
}
Status: UNCONFIRMED → NEW
Ever confirmed: true
Once I fix and land the bug 444279 which will introduce mochitest for name calculations then I'll take this bug.
Assignee: nobody → surkov.alexander
Attached patch patch (obsolete) — Splinter Review
Attachment #329782 - Flags: review?(aaronleventhal)
Comment on attachment 329782 [details] [diff] [review]
patch

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">diff --git a/accessible/src/base/nsAccessible.cpp b/accessible/src/base/nsAccessible.cpp
>--- a/accessible/src/base/nsAccessible.cpp
>+++ b/accessible/src/base/nsAccessible.cpp
>@@ -1588,25 +1588,16 @@
>       }
>     }
>     return NS_OK;
>   }
> 
>   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;
>-      }
>-

this code has been introduced in bug 306875. I can't reproduce this bug any more to include mochitests for this. Probably something has changed. Though I'm sure this code isn't needed any more.
Attachment #329782 - Flags: review?(marco.zehe)
Status: NEW → ASSIGNED
Flags: in-testsuite?
Comment on attachment 329782 [details] [diff] [review]
patch

r=me for the test parts with the following nits/suggestions:
+      // (label element is hidden entirerly)

This is in both the HTML and XUL files. The word is spelled "entirely" (omitt the r before the l).

Also, please include an explicit test for the original bug report. In there, the accName was desired not to contain the hidden span within the option. I don't see you doing that, you only test for non-hidden with the explicit labelled by. However I'd prefer if we had a test for the actual testcase as well. Thanks!
Attachment #329782 - Flags: review?(marco.zehe) → review+
If you like then I'll include original test. Though that name calculation and name calculation from aria-labelledby uses the same functions.
(In reply to comment #7)
> If you like then I'll include original test. Though that name calculation and
> name calculation from aria-labelledby uses the same functions.

Yes, but the expected result is different. See original bug report in comment #0. If not aria-labelledby, hidden child should be *excluded* from the acc name.
Attached patch patch2Splinter Review
Attachment #329782 - Attachment is obsolete: true
Attachment #329792 - Flags: review?(aaronleventhal)
Attachment #329782 - Flags: review?(aaronleventhal)
Comment on attachment 329792 [details] [diff] [review]
patch2

Code looks good. Do you think some of the tests should use visibility: hidden instead of only testing display: none?

Either way r+=aaronlev
Attachment #329792 - Flags: review?(aaronleventhal) → review+
checked in with Aaron's example (visibility: hidden)

http://hg.mozilla.org/mozilla-central/index.cgi/rev/9e360506ae23
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
You need to log in before you can comment on or make changes to this bug.