Closed Bug 286144 Opened 17 years ago Closed 17 years ago
Support DHTML listbox accessibility
This bug is for supporting listbox and listitem widgets with the new dynamic content accessibility code.
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)
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.
> 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).
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 :)
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: 17 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.