Problems returning accessible option children of combo box's list

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({access, sec508})

Trunk
x86
All
access, sec508
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

14 years ago
Offshoot of bug 278034.

While testing bug 278034 I noticed we're reporting about twice as many option
accessibles as there should be. We're also having trouble returning the actual
option accessibles.

This seems to be due to the fact that empty text nodes are now in between the
option nodes -- I don't remember this being the case in that past.

In any case this code needs to be rewritten to deal well with the accessibility
caching we now have.
(Assignee)

Comment 1

14 years ago
Created attachment 171659 [details] [diff] [review]
Override CacheChildren(), but don't override GetFirstChild(), GetNextSibling(), GetChildCount() etc.
(Assignee)

Updated

14 years ago
Attachment #171659 - Flags: review?(Louie.Zhao)

Comment 2

14 years ago
Comment on attachment 171659 [details] [diff] [review]
Override CacheChildren(), but don't override GetFirstChild(), GetNextSibling(), GetChildCount() etc.

In mozilla 1.7 branch, I didn't see this issue. The patch looks good and has
been verified on ATK.
Attachment #171659 - Flags: review?(Louie.Zhao) → review+

Updated

14 years ago
Attachment #171659 - Flags: superreview?(Henry.Jia)

Updated

14 years ago
Blocks: 278034
(Assignee)

Comment 3

14 years ago
Sylvain independently ran into this problem and is blocked by it.
(Assignee)

Comment 4

14 years ago
This regression was caused bug bug 240139.
Note that this could have been caused before bug 240139 as well, simply by
constructing <select>s via the DOM and inserting random child nodes.  The new
code is much more robust, I'm glad to see, and should be able to deal with this.

Note that the code, as written, will pick up all sorts of gunk in XHTML
documents that may not actually be HTML.  I'd suggest IsContentOfType() checks
in addition to the Tag() checks.  I realize that accessibility doesn't
necessarily support XHTML yet, but starting to build the groundwork for doing it
now will make it easier later...
(Assignee)

Comment 6

14 years ago
Created attachment 172363 [details] [diff] [review]
Add IsContentOfType check
Attachment #171659 - Attachment is obsolete: true
Attachment #172363 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

14 years ago
Attachment #171659 - Flags: superreview?(Henry.Jia)
Comment on attachment 172363 [details] [diff] [review]
Add IsContentOfType check

>Index: accessible/src/html/nsHTMLSelectAccessible.cpp
nsHTMLSelectListAccessible::AccessibleForOption(nsIAccessibilityService 
>+  nsIAccessible *accessible;
>+  aAccService->GetAccessibleInWeakShell(domNode, mWeakShell, &accessible);
>+  nsCOMPtr<nsPIAccessible> privateAccessible(do_QueryInterface(accessible));
>+  if (privateAccessible) {

And if not we leak "accessible"?

This function should clearly document that it outs an addrefed element.  In
fact, it should just return an already_AddRefed instead of using an out param.

>+void nsHTMLSelectListAccessible::CacheChildren(PRBool aWalkAnonContent)
>+      AccessibleForOption(accService, childContent, &lastGoodAccessible);

So this leaks.

>+        nsIContent *grandChildContent = selectContent->GetChildAt(innerCount);

Note that we may well end up implementing nested optgroups in the near (next
year?) future.

Marking sr- due to the memory leaks...
Attachment #172363 - Flags: superreview?(bzbarsky) → superreview-
(Assignee)

Comment 8

14 years ago
Created attachment 172371 [details] [diff] [review]
Use already_AddRefed<> and handle nested optgroups via recursion

Will all compilers handle this the same? It's working under Windows. Or, do I
need a temporary variable.

nsCOMPtr<nsIAccessible> lastGoodAccessible;
...
for () {
      lastGoodAccessible = AccessibleForOption(aAccService,
					       childContent,
					       lastGoodAccessible);
}
Attachment #172363 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #172371 - Flags: superreview?(bzbarsky)
Comment on attachment 172371 [details] [diff] [review]
Use already_AddRefed<> and handle nested optgroups via recursion

Yeah, compilers should handle this fine.

>+void nsHTMLSelectListAccessible::CacheChildren(PRBool aWalkAnonContent)
>+  CacheOptSiblings(accService, selectContent, nsnull);

This leaks.  You need to assign the return value into an nsCOMPtr, so it'll get
released.

sr=bzbarsky with that change.
Attachment #172371 - Flags: superreview?(bzbarsky) → superreview+
This broke the tree:

nsHTMLSelectAccessible.cpp: In method `class nsIFrame *
nsHTMLSelectOptionAccessible::GetBoundsFrame()':
nsHTMLSelectAccessible.cpp:521: no matching function for call to
`nsDerivedSafe<nsIAccessible>::GetFinalState (PRUint32 *)'
nsHTMLSelectAccessible.cpp: In method `nsresult
nsHTMLComboboxAccessible::GetValue(class nsAString &)':
nsHTMLSelectAccessible.cpp:882: no matching function for call to
`nsDerivedSafe<nsIAccessible>::GetFinalValue (nsAString &)'
nsHTMLSelectAccessible.cpp:883: warning: control reaches end of non-void
function `nsHTMLComboboxAccessible::GetValue(nsAString &)'
I backed out the last two hunks of this patch on the assumption that they'd
crept into the diff accidentally.

Updated

14 years ago
Attachment #172371 - Flags: review+

Comment 12

14 years ago
Patch already been checked in. Mark it fixed.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.