Closed
Bug 443081
Opened 17 years ago
Closed 17 years ago
ARIA name from child content is incorrectly including child elements that have display:none
Categories
(Core :: Disability Access APIs, defect)
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)
|
(deleted),
patch
|
aaronlev
:
review+
|
Details | Diff | Splinter Review |
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"
|
||
Updated•17 years ago
|
|
||
Comment 1•17 years ago
|
||
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
|
|
||
Comment 2•17 years ago
|
||
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
|
Assignee | |
Comment 3•17 years ago
|
||
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
|
|
Assignee | |
Comment 4•17 years ago
|
||
Attachment #329782 -
Flags: review?(aaronleventhal)
|
|
Assignee | |
Comment 5•17 years ago
|
||
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)
|
|
Assignee | |
Updated•17 years ago
|
Status: NEW → ASSIGNED
|
|
Assignee | |
Updated•17 years ago
|
Flags: in-testsuite?
|
|
||
Comment 6•17 years ago
|
||
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+
|
|
Assignee | |
Comment 7•17 years ago
|
||
If you like then I'll include original test. Though that name calculation and name calculation from aria-labelledby uses the same functions.
|
|
||
Comment 8•17 years ago
|
||
(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.
|
|
Assignee | |
Comment 9•17 years ago
|
||
Attachment #329782 -
Attachment is obsolete: true
Attachment #329792 -
Flags: review?(aaronleventhal)
Attachment #329782 -
Flags: review?(aaronleventhal)
|
|
||
Comment 10•17 years ago
|
||
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+
|
|
||
Comment 11•17 years ago
|
||
One more small thing, can you change here:
http://developer.mozilla.org/en/docs/ARIA_User_Agent_Implementors_Guide#11.3.6_Name
|
|
Assignee | |
Comment 12•17 years ago
|
||
checked in with Aaron's example (visibility: hidden)
http://hg.mozilla.org/mozilla-central/index.cgi/rev/9e360506ae23
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
|
|
||
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.1a1
You need to log in
before you can comment on or make changes to this bug.
Description
•