Support DHTML listbox accessibility

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, 3 obsolete attachments)

(Assignee)

Description

14 years ago
This bug is for supporting listbox and listitem widgets with the new dynamic
content accessibility code.
(Assignee)

Updated

14 years ago
Blocks: 286145
(Assignee)

Comment 1

14 years ago
Created attachment 177426 [details] [diff] [review]
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.
Attachment #177426 - Flags: review?(pkwarren)

Comment 2

14 years ago
Why we need to add |IsFocusableExternal|?
(Assignee)

Comment 3

14 years ago
> 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().
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.
(Assignee)

Comment 5

14 years ago
> 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.
(Assignee)

Updated

14 years ago
Attachment #177426 - Flags: review?(pkwarren) → review?(timeless)
(Assignee)

Comment 6

14 years ago
Created 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)
Attachment #177426 - Attachment is obsolete: true
Attachment #177871 - Flags: superreview?(bzbarsky)
Attachment #177871 - Flags: review?(timeless)
(Assignee)

Updated

14 years ago
Attachment #177426 - Flags: review?(timeless)
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).
(Assignee)

Comment 8

14 years ago
(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 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?
Attachment #177871 - Flags: superreview?(bzbarsky) → superreview-
(Assignee)

Comment 10

14 years ago
Created attachment 178360 [details] [diff] [review]
Should not have passed in pointer of PRUint32

Scary that the last thing compiled and ran correctly through my tests.
Attachment #177871 - Attachment is obsolete: true
Attachment #178360 - Flags: superreview?(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).
(Assignee)

Updated

14 years ago
Attachment #177871 - Flags: review?(timeless)
(Assignee)

Updated

14 years ago
Attachment #178360 - Flags: review?(timeless)
(Assignee)

Comment 12

14 years ago
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.
(Assignee)

Comment 13

14 years ago
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
(Assignee)

Comment 14

14 years ago
Created attachment 179062 [details] [diff] [review]
Get rid of IsFocusableExternal() -- just use virtual IsFocusable()

Timeless the IsFocusableExternal() issue isn't blocking your r= anymore :)
Attachment #178360 - Attachment is obsolete: true
Attachment #179062 - Flags: superreview?(bzbarsky)
Attachment #179062 - Flags: review?(timeless)

Updated

14 years ago
Attachment #178360 - Flags: superreview?(bzbarsky)
Attachment #178360 - Flags: review?(timeless)

Updated

14 years ago
Attachment #179062 - Flags: review?(timeless) → review+
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.
Attachment #179062 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 16

14 years ago
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Looks like changing IsFocusable to virtual regressed Tp a little bit on btek.
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...
Maybe it was just btek having a spasm.
You need to log in before you can comment on or make changes to this bug.