286144 - Support DHTML listbox accessibility

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

This bug is for supporting listbox and listitem widgets with the new dynamic
content accessibility code.

Comment 1

[Aaron Leventhal]

Attachment: 1) Support list and listitem roles, 2) Allow states to be turned off by setting them to false, 3) Allow XUL use of role+state by fixing focusable state check & firing dynamic content events in it

This patch is preparing for bug 286145 where we're going to use the list and
listitem roles to fix the way we expose the new preference category list that
uses styled radio buttons.

Comment 2

[Ginn Chen]

Why we need to add |IsFocusableExternal|?

Comment 3

[Aaron Leventhal]

> Why we need to add |IsFocusableExternal|?

I'm not sure why we need Foo() and FooExternal() in nsIFrame.

I ran into linker errors when I didn't do it that way. We do the same thing for
other nsIFrame methods that are used outside of core Gecko, for example
GetView() and GetViewExternal().

Comment 4

[Boris Zbarsky [:bzbarsky]]

You need *External versions of non-virtual and non-inline nsIFrame methods
(because being non-virtual, they cannot be called from outside the module).

But the *External methods then need to be focusable, of course... which this is not.

Comment 5

[Aaron Leventhal]

> But the *External methods then need to be focusable, of course... which this
is not.

I don't get it. How can a method be focusable? That was either a typo or I need
more coffee.

Comment 6

[Aaron Leventhal]

Attachment: Fixes for Timeless' comments on IRC. He wants bz to look at it now (especially to weigh in the IsFocusableExternal() issue)

Comment 7

[Boris Zbarsky [:bzbarsky]]

That was a typo.  I meant "virtual", not "focusable"....  I don't really see how
the code, as written, can link and run, in my rude understanding of library
linking and calling conventions... (but again, it's rude understanding).

Comment 8

[Aaron Leventhal]

(In reply to comment #7)
+  virtual PRBool IsFocusableExternal(PRInt32 *aTabIndex = nsnull, PRBool
aWithMouse = PR_FALSE);

But the IsFocusableExternal() method is virtual, which is why it can link. The
external callers use that, and callers inside core Gecko use the non-virtual
IsFocusable().

How is this any different from GetView()/GetViewExternal() ?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177871 [details] [diff] [review]
Fixes for Timeless' comments on IRC. He wants bz to look at it now (especially to weigh in the IsFocusableExternal() issue)

>Index: accessible/src/base/nsAccessible.cpp
>+    finalState = MappedAttrState(content, &finalState, 
&mRoleMapEntry->attributeMap1);

The second arg should be a PRUint32, not a pointer, right?  This code won't
compile on the 64-bit platforms, and looks wrong to me...

>Index: accessible/src/base/nsAccessible.h
>+  PRUint32 MappedAttrState(nsIContent *aContent, PRUint32 aStartState,nsStateMapEntry *aStateMapEntry);

Space after comma?

Comment 10

[Aaron Leventhal]

Attachment: Should not have passed in pointer of PRUint32

Scary that the last thing compiled and ran correctly through my tests.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Scary that the last thing compiled and ran correctly through my tests.

It probably warned at compile time, you know....

Anyway, it'll be some time before I can get to this (order of a week or more; I
have long-promised reviews for bryner that I need to do).

Comment 12

[Aaron Leventhal]

Actually we just need one virtual IsFocusable()

I need to make it virtual anyway because I'm planning to override it for
nsObjectFrame so that they are not focusable for broken plugins.

Comment 13

[Aaron Leventhal]

Do I need to post a new patch that only uses a virtual IsFocusable() and has no
IsFocusableExternal(), or can that just be a condition of checkin?

I also have the change to the virtual IsFocusable() in bug 245349

Comment 14

[Aaron Leventhal]

Attachment: Get rid of IsFocusableExternal() -- just use virtual IsFocusable()

Timeless the IsFocusableExternal() issue isn't blocking your r= anymore :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179062 [details] [diff] [review]
Get rid of IsFocusableExternal() -- just use virtual IsFocusable()

>Index: accessible/src/base/nsAccessible.cpp
>+    if (NS_ConvertUCS2toUTF8(attribValue).Equals(aStateMapEntry->attributeValue)) 

UTF16, please, not UCS2.

> NS_IMETHODIMP nsAccessible::GetFinalState(PRUint32 *aState)

>+      nsIAccessible *current = container;

That needs to be a strong pointer, per COM rules.  I suggest:

 nsCOMPtr<nsIAccessible> current;
 current.swap(container);

to avoid extra addrefs/releases.

sr=bzbarsky with those nits.

Comment 16

[Aaron Leventhal]

Checking in src/base/nsAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.141; previous revision: 1.140
done
Checking in src/base/nsAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsAccessible.h,v  <--  nsAccessible.h
new revision: 1.58; previous revision: 1.57
done
Checking in src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <-- 
nsRootAccessible.cpp
new revision: 1.113; previous revision: 1.112
done

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks like changing IsFocusable to virtual regressed Tp a little bit on btek.

Comment 18

[Boris Zbarsky [:bzbarsky]]

It was virtual before this patch got checked in (ever since the patch for bug
245349 landed)... So if this patch had an effect, it was something else...

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe it was just btek having a spasm.

Comment 20

[Firefox Bug Husbandry Bot]

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