Closed Bug 286144 Opened 15 years ago Closed 15 years ago

Support DHTML listbox accessibility

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

(Keywords: access)

Attachments

(1 file, 3 obsolete files)

This bug is for supporting listbox and listitem widgets with the new dynamic
content accessibility code.
Blocks: 286145
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)
Why we need to add |IsFocusableExternal|?
> 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.
> 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.
Attachment #177426 - Flags: review?(pkwarren) → review?(timeless)
Attachment #177426 - Attachment is obsolete: true
Attachment #177871 - Flags: superreview?(bzbarsky)
Attachment #177871 - Flags: review?(timeless)
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).
(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-
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).
Attachment #177871 - Flags: review?(timeless)
Attachment #178360 - Flags: review?(timeless)
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.
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
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)
Attachment #178360 - Flags: superreview?(bzbarsky)
Attachment #178360 - Flags: review?(timeless)
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+
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
Closed: 15 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.