278872 - Problems returning accessible option children of combo box's list

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

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.

Comment 1

[Aaron Leventhal]

Attachment: Override CacheChildren(), but don't override GetFirstChild(), GetNextSibling(), GetChildCount() etc.

Comment 2

[Louie Zhao]

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.

Comment 3

[Aaron Leventhal]

Sylvain independently ran into this problem and is blocked by it.

Comment 4

[Aaron Leventhal]

This regression was caused bug bug 240139.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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...

Comment 6

[Aaron Leventhal]

Attachment: Add IsContentOfType check

Comment 7

[Boris Zbarsky [:bzbarsky]]

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...

Comment 8

[Aaron Leventhal]

Attachment: 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);
}

Comment 9

[Boris Zbarsky [: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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 &)'

Comment 11

[Boris Zbarsky [:bzbarsky]]

I backed out the last two hunks of this patch on the assumption that they'd
crept into the diff accidentally.

Comment 12

[Louie Zhao]

Patch already been checked in. Mark it fixed.

Comment 13

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381